classification
Title: improved PEP 409 implementation
Type: Stage:
Components: Interpreter Core Versions: Python 3.3
process
Status: closed Resolution: fixed
Dependencies: Superseder:
Assigned To: Nosy List: Arfrever, benjamin.peterson, georg.brandl, ncoghlan, pitrou, poke, python-dev
Priority: normal Keywords: patch

Created on 2012-02-26 15:19 by benjamin.peterson, last changed 2012-06-28 21:11 by poke. This issue is now closed.

Files
File name Uploaded Description Edit
pep409.patch benjamin.peterson, 2012-02-26 15:19 review
pep409.patch benjamin.peterson, 2012-02-26 20:01 review
pep415.patch benjamin.peterson, 2012-02-27 16:07 review
pep415.patch benjamin.peterson, 2012-02-28 00:17 review
pep415.patch benjamin.peterson, 2012-03-05 21:50 review
Messages (29)
msg154358 - (view) Author: Benjamin Peterson (benjamin.peterson) * (Python committer) Date: 2012-02-26 15:19
It uses a __suppress_context__ attribute rather than the whole Ellipsis mess.
msg154359 - (view) Author: Antoine Pitrou (pitrou) * (Python committer) Date: 2012-02-26 15:25
I don't think it has to be a __special__ attribute. suppress_context would probably be fine.
Also, instead of looking up the attribute in the dict, why not have a dedicated C member?
msg154373 - (view) Author: Benjamin Peterson (benjamin.peterson) * (Python committer) Date: 2012-02-26 17:14
__suppress_context__ parallels with __cause__ and __context__.
msg154380 - (view) Author: Benjamin Peterson (benjamin.peterson) * (Python committer) Date: 2012-02-26 20:01
Here's a patch which doesn't use the dict.
msg154387 - (view) Author: Nick Coghlan (ncoghlan) * (Python committer) Date: 2012-02-26 20:54
I don't consider using Ellipsis as a sentinel value a mess. If you don't like the PEP's solution, take it up with Guido.
msg154388 - (view) Author: Benjamin Peterson (benjamin.peterson) * (Python committer) Date: 2012-02-26 20:55
Maybe it's not awful, but why is this not a cleaner solution?
msg154391 - (view) Author: Antoine Pitrou (pitrou) * (Python committer) Date: 2012-02-26 21:16
I find this cleaner than the contrived "Ellipsis as magic value" solution, myself :)
msg154392 - (view) Author: Nick Coghlan (ncoghlan) * (Python committer) Date: 2012-02-26 21:18
Because you're breaking the semantics of the "raise X from Y" syntax.

That syntax is *just* syntactic sugar for "_exc = X; _exc.__cause__ = Y; raise _exc".

Under PEP 409, that remains true.

Your patch breaks it.

If you want to change the meaning for "raise X from Y", write a new PEP.
msg154394 - (view) Author: Nick Coghlan (ncoghlan) * (Python committer) Date: 2012-02-26 21:27
The other problem with the "separate attribute" approach is that it makes the condition for suppressing the context ugly:

  exc.__suppress_context__ or exc.__cause__ is not None

That's hardly cleaner than:

  exc.__cause__ is not Ellipsis
msg154395 - (view) Author: Antoine Pitrou (pitrou) * (Python committer) Date: 2012-02-26 21:29
> The other problem with the "separate attribute" approach is that it
> makes the condition for suppressing the context ugly:
> 
>   exc.__suppress_context__ or exc.__cause__ is not None
> 
> That's hardly cleaner than:
> 
>   exc.__cause__ is not Ellipsis

I find it cleaner actually, because it states the intent rather than
hiding it behing an unexpected magic value (Ellipsis not being
exception-related).
msg154396 - (view) Author: Nick Coghlan (ncoghlan) * (Python committer) Date: 2012-02-26 21:31
Regardless, I'm rejecting this for not complying with the PEP specification (which is quite explicit about the implementation mechanism and the API exposed to Python). If you want to challenge an approved PEP, the correct way to go about it is to write a new PEP.
msg154403 - (view) Author: Georg Brandl (georg.brandl) * (Python committer) Date: 2012-02-26 21:59
FWIW, I agree with Nick: once we go as far as writing PEPs for smaller features, we should keep to the spec as accepted.
msg154484 - (view) Author: Benjamin Peterson (benjamin.peterson) * (Python committer) Date: 2012-02-27 16:07
Now with doc changes!
msg154523 - (view) Author: Benjamin Peterson (benjamin.peterson) * (Python committer) Date: 2012-02-28 02:16
I don't think it's a good idea for setting one attribute to implicitly set another.
msg154526 - (view) Author: Nick Coghlan (ncoghlan) * (Python committer) Date: 2012-02-28 02:28
The alternatives are a backwards compatibility break (i.e. raise exc from other_exc would suppress the context, but exc.__cause__ = other_exc would not) or else that we don't succeed in eliminating the dual use of __cause__ in the display routines.

Given that those two problems are the reason I went for the PEP 409 approach in the first place, I'm really only interested in alternative approaches that eliminate them (such as setting the flag automatically whenever __cause__ is set). If you don't like it, then we already have PEP 409's Ellipsis based implementation which meets my criteria.
msg154527 - (view) Author: Nick Coghlan (ncoghlan) * (Python committer) Date: 2012-02-28 02:30
Also, ensuring class invariants by setting derived attributes correctly is one of the primary use cases for properties, so objecting to my proposed approach is objecting to a fairly fundamental programming technique.
msg154981 - (view) Author: Benjamin Peterson (benjamin.peterson) * (Python committer) Date: 2012-03-05 21:50
Now with implicit setting of __suppress_context__!
msg155433 - (view) Author: Benjamin Peterson (benjamin.peterson) * (Python committer) Date: 2012-03-12 02:53
Nick, care to look at the latest patch?
msg155438 - (view) Author: Nick Coghlan (ncoghlan) * (Python committer) Date: 2012-03-12 04:05
Reviewed - actual impl looks good to me, couple of comments regarding the docs and tests.
msg155853 - (view) Author: Benjamin Peterson (benjamin.peterson) * (Python committer) Date: 2012-03-15 04:26
I'm wondering if allowing printing __context__ and __cause__ is the best idea. Both could have chains and if they do it will be very non-obvious which one came from which.
msg156016 - (view) Author: Nick Coghlan (ncoghlan) * (Python committer) Date: 2012-03-16 12:24
With the decision on whether or not to suppress the context moved out to a separate flag, I think we need to allow it. Requiring that the flag be False *and* that the context also be None gets us back to asking the question of why the flag doesn't work when the context is set to a different value. That question was part of the genesis of the Ellipsis-as-sentinel approach in the original 409 implementation.

I wouldn't stress too much about the formatting though. Perhaps note in the suppress context docs that the output can get *very* noisy if you turn it off when both cause and context are set, so while it does display all the exception information, the default output isn't going to be very easy to read.
msg160611 - (view) Author: Nick Coghlan (ncoghlan) * (Python committer) Date: 2012-05-14 12:13
I have accepted the PEP.

Issue 14805 now covers the separate question of allowing both __cause__ and __context__ to be displayed in the same traceback.
msg160682 - (view) Author: Roundup Robot (python-dev) Date: 2012-05-15 05:09
New changeset b0eb7d2a9542 by Benjamin Peterson in branch 'default':
PEP 415: Implement suppression of __context__ display with an exception attribute
http://hg.python.org/cpython/rev/b0eb7d2a9542
msg160683 - (view) Author: Benjamin Peterson (benjamin.peterson) * (Python committer) Date: 2012-05-15 05:10
Thanks for the review/dictating, Nick!
msg160689 - (view) Author: Georg Brandl (georg.brandl) * (Python committer) Date: 2012-05-15 07:01
I hope you're not disappointed when that PEP doesn't show up in the release notes :)
msg160736 - (view) Author: Benjamin Peterson (benjamin.peterson) * (Python committer) Date: 2012-05-15 15:33
2012/5/15 Georg Brandl <report@bugs.python.org>:
>
> Georg Brandl <georg@python.org> added the comment:
>
> I hope you're not disappointed when that PEP doesn't show up in the release notes :)

It gives me more peace of mind than any release note ever could. :)
msg164229 - (view) Author: Patrick Westerhoff (poke) Date: 2012-06-28 08:58
Hey, I just saw the release notes for 3.3 and would like a quick confirmation: This is included in 3.3, right? ^^
msg164266 - (view) Author: Nick Coghlan (ncoghlan) * (Python committer) Date: 2012-06-28 15:53
Yep - note that PEP 409 was updated to say that the the implementation discussion has been superceded by PEP 415. It may be worth making that note more prominent, though...
msg164293 - (view) Author: Patrick Westerhoff (poke) Date: 2012-06-28 21:11
Alright, thought so but wanted a confirmation anyway – thanks a lot :D
History
Date User Action Args
2012-06-28 21:11:31pokesetmessages: + msg164293
2012-06-28 15:53:58ncoghlansetmessages: + msg164266
2012-06-28 08:58:35pokesetmessages: + msg164229
2012-05-15 15:33:17benjamin.petersonsetmessages: + msg160736
2012-05-15 07:01:42georg.brandlsetmessages: + msg160689
2012-05-15 05:10:57benjamin.petersonsetstatus: open -> closed
resolution: fixed
messages: + msg160683
2012-05-15 05:09:56python-devsetnosy: + python-dev
messages: + msg160682
2012-05-14 12:13:20ncoghlansetmessages: + msg160611
2012-05-14 12:12:23ncoghlanlinkissue14805 dependencies
2012-03-16 12:24:27ncoghlansetmessages: + msg156016
2012-03-15 04:26:14benjamin.petersonsetmessages: + msg155853
2012-03-12 04:05:40ncoghlansetmessages: + msg155438
2012-03-12 02:53:39benjamin.petersonsetmessages: + msg155433
2012-03-05 21:50:14benjamin.petersonsetfiles: + pep415.patch

messages: + msg154981
2012-03-02 13:56:01pokesetnosy: + poke
2012-02-28 02:30:40ncoghlansetmessages: + msg154527
2012-02-28 02:28:41ncoghlansetmessages: + msg154526
2012-02-28 02:16:33benjamin.petersonsetmessages: + msg154523
2012-02-28 00:17:14benjamin.petersonsetstatus: closed -> open
files: + pep415.patch
resolution: rejected -> (no value)
2012-02-27 16:07:36benjamin.petersonsetfiles: + pep415.patch

messages: + msg154484
2012-02-26 21:59:24georg.brandlsetnosy: + georg.brandl
messages: + msg154403
2012-02-26 21:31:53ncoghlansetstatus: open -> closed
resolution: rejected
messages: + msg154396
2012-02-26 21:29:48pitrousetmessages: + msg154395
2012-02-26 21:27:51ncoghlansetmessages: + msg154394
2012-02-26 21:18:47ncoghlansetmessages: + msg154392
2012-02-26 21:16:21pitrousetmessages: + msg154391
2012-02-26 20:55:47benjamin.petersonsetmessages: + msg154388
2012-02-26 20:54:36ncoghlansetmessages: + msg154387
2012-02-26 20:01:28benjamin.petersonsetfiles: + pep409.patch

messages: + msg154380
2012-02-26 18:02:50Arfreversetnosy: + Arfrever
2012-02-26 17:14:44benjamin.petersonsetmessages: + msg154373
2012-02-26 15:25:53pitrousetnosy: + pitrou
messages: + msg154359
2012-02-26 15:19:22benjamin.petersoncreate