Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

improved PEP 409 implementation #58341

Closed
benjaminp opened this issue Feb 26, 2012 · 29 comments
Closed

improved PEP 409 implementation #58341

benjaminp opened this issue Feb 26, 2012 · 29 comments
Labels
interpreter-core (Objects, Python, Grammar, and Parser dirs)

Comments

@benjaminp
Copy link
Contributor

BPO 14133
Nosy @birkenfeld, @ncoghlan, @pitrou, @benjaminp
Files
  • pep409.patch
  • pep409.patch
  • pep415.patch
  • pep415.patch
  • pep415.patch
  • Note: these values reflect the state of the issue at the time it was migrated and might not reflect the current state.

    Show more details

    GitHub fields:

    assignee = None
    closed_at = <Date 2012-05-15.05:10:57.376>
    created_at = <Date 2012-02-26.15:19:22.248>
    labels = ['interpreter-core']
    title = 'improved PEP 409 implementation'
    updated_at = <Date 2012-06-28.21:11:31.870>
    user = 'https://github.com/benjaminp'

    bugs.python.org fields:

    activity = <Date 2012-06-28.21:11:31.870>
    actor = 'poke'
    assignee = 'none'
    closed = True
    closed_date = <Date 2012-05-15.05:10:57.376>
    closer = 'benjamin.peterson'
    components = ['Interpreter Core']
    creation = <Date 2012-02-26.15:19:22.248>
    creator = 'benjamin.peterson'
    dependencies = []
    files = ['24648', '24651', '24664', '24667', '24742']
    hgrepos = []
    issue_num = 14133
    keywords = ['patch']
    message_count = 29.0
    messages = ['154358', '154359', '154373', '154380', '154387', '154388', '154391', '154392', '154394', '154395', '154396', '154403', '154484', '154523', '154526', '154527', '154981', '155433', '155438', '155853', '156016', '160611', '160682', '160683', '160689', '160736', '164229', '164266', '164293']
    nosy_count = 7.0
    nosy_names = ['georg.brandl', 'ncoghlan', 'pitrou', 'benjamin.peterson', 'Arfrever', 'poke', 'python-dev']
    pr_nums = []
    priority = 'normal'
    resolution = 'fixed'
    stage = None
    status = 'closed'
    superseder = None
    type = None
    url = 'https://bugs.python.org/issue14133'
    versions = ['Python 3.3']

    @benjaminp
    Copy link
    Contributor Author

    It uses a __suppress_context__ attribute rather than the whole Ellipsis mess.

    @benjaminp benjaminp added the interpreter-core (Objects, Python, Grammar, and Parser dirs) label Feb 26, 2012
    @pitrou
    Copy link
    Member

    pitrou commented Feb 26, 2012

    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?

    @benjaminp
    Copy link
    Contributor Author

    __suppress_context__ parallels with __cause__ and __context__.

    @benjaminp
    Copy link
    Contributor Author

    Here's a patch which doesn't use the dict.

    @ncoghlan
    Copy link
    Contributor

    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.

    @benjaminp
    Copy link
    Contributor Author

    Maybe it's not awful, but why is this not a cleaner solution?

    @pitrou
    Copy link
    Member

    pitrou commented Feb 26, 2012

    I find this cleaner than the contrived "Ellipsis as magic value" solution, myself :)

    @ncoghlan
    Copy link
    Contributor

    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.

    @ncoghlan
    Copy link
    Contributor

    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

    @pitrou
    Copy link
    Member

    pitrou commented Feb 26, 2012

    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).

    @ncoghlan
    Copy link
    Contributor

    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.

    @birkenfeld
    Copy link
    Member

    FWIW, I agree with Nick: once we go as far as writing PEPs for smaller features, we should keep to the spec as accepted.

    @benjaminp
    Copy link
    Contributor Author

    Now with doc changes!

    @benjaminp benjaminp reopened this Feb 28, 2012
    @benjaminp
    Copy link
    Contributor Author

    I don't think it's a good idea for setting one attribute to implicitly set another.

    @ncoghlan
    Copy link
    Contributor

    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.

    @ncoghlan
    Copy link
    Contributor

    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.

    @benjaminp
    Copy link
    Contributor Author

    Now with implicit setting of __suppress_context__!

    @benjaminp
    Copy link
    Contributor Author

    Nick, care to look at the latest patch?

    @ncoghlan
    Copy link
    Contributor

    Reviewed - actual impl looks good to me, couple of comments regarding the docs and tests.

    @benjaminp
    Copy link
    Contributor Author

    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.

    @ncoghlan
    Copy link
    Contributor

    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.

    @ncoghlan
    Copy link
    Contributor

    I have accepted the PEP.

    bpo-14805 now covers the separate question of allowing both __cause__ and __context__ to be displayed in the same traceback.

    @python-dev
    Copy link
    Mannequin

    python-dev mannequin commented May 15, 2012

    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

    @benjaminp
    Copy link
    Contributor Author

    Thanks for the review/dictating, Nick!

    @birkenfeld
    Copy link
    Member

    I hope you're not disappointed when that PEP doesn't show up in the release notes :)

    @benjaminp
    Copy link
    Contributor Author

    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. :)

    @poke
    Copy link
    Mannequin

    poke mannequin commented Jun 28, 2012

    Hey, I just saw the release notes for 3.3 and would like a quick confirmation: This is included in 3.3, right? ^^

    @ncoghlan
    Copy link
    Contributor

    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...

    @poke
    Copy link
    Mannequin

    poke mannequin commented Jun 28, 2012

    Alright, thought so but wanted a confirmation anyway – thanks a lot :D

    @ezio-melotti ezio-melotti transferred this issue from another repository Apr 10, 2022
    Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
    Labels
    interpreter-core (Objects, Python, Grammar, and Parser dirs)
    Projects
    None yet
    Development

    No branches or pull requests

    4 participants