gh-89770: Implement PEP-678 - Exception notes by iritkatriel · Pull Request #31317 · python/cpython (original) (raw)
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 andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.Learn more about bidirectional Unicode characters
[ Show hidden characters]({{ revealButtonHref }})
…le of the assigned notes
🤖 New build scheduled with the buildbot fleet by @iritkatriel for commit 1555087 🤖
If you want to schedule another build, you need to add the "🔨 test-with-buildbots" label again.
This was referenced
Feb 26, 2022
PEP-678: exception notes are set by add_note(). __notes__ holds a tuple of the assigned notes PEP-678: implementation of exception notes as per PEP discussions
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Where's the descriptor for "__notes__"?
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
"Errors should never pass silently."
How about error handling when printing the traceback? Should we print something like "cannot print notes: not a sequence", and similarly if a note is not a str?
How about error handling when printing the traceback? Should we print something like "cannot print notes: not a sequence", and similarly if a note is not a str?
How about printing repr(self.__notes__) if it's not a list, and str(note) if an individual note isn't a string? The former makes it clear what it is, while the latter provides a feature that Barry has been asking for...
If either of those fails, either raise an exception from print_exception() or swallow the error and print a simple message. It probably doesn't really matter, repr() or str() failing should be extremely obscure.
How about error handling when printing the traceback? Should we print something like "cannot print notes: not a sequence", and similarly if a note is not a str?
How about printing
repr(self.__notes__)if it's not a list, andstr(note)if an individual note isn't a string? The former makes it clear what it is, while the latter provides a feature that Barry has been asking for...If either of those fails, either raise an exception from
print_exception()or swallow the error and print a simple message. It probably doesn't really matter,repr()orstr()failing should be extremely obscure.
I'll do something like what we do for str(exc).
… string, print str(note)
Heya - I think we should have an explicit test that if you .split() an ExceptionGroup which already has __notes__, it's shallow-copied rather than simply assigned to the new instances. It'd be pretty confusing to call .add_note() on one and have it added to other instances too!
Heya - I think we should have an explicit test that if you
.split()anExceptionGroupwhich already has__notes__, it's shallow-copied rather than simply assigned to the new instances. It'd be pretty confusing to call.add_note()on one and have it added to other instances too!
That's a good point. I added the test and fixed the code to copy the notes in split(), so long as they are a sequence (producing a list). If people assign arbitrary objects to notes I don't know how to copy them.
Is there a "copy protocol" we can demand?
gh-91479: Implement PEP-678 - Exception notes bpo-45607: Implement PEP-678 - Exception notes
bpo-45607: Implement PEP-678 - Exception notes gh-89770: Implement PEP-678 - Exception notes
🤖 New build scheduled with the buildbot fleet by @iritkatriel for commit 95be670 🤖
If you want to schedule another build, you need to add the "🔨 test-with-buildbots" label again.
Sorry about the noise (I messed it up multitasking). Getting there now.
🤖 New build scheduled with the buildbot fleet by @iritkatriel for commit 602b4c4 🤖
If you want to schedule another build, you need to add the "🔨 test-with-buildbots" label again.
The buildbots are happy with this. Let me know if you would like to re-review, otherwise I'll merge it tomorrow or so.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| return NULL; |
|---|
| } |
| if (!PyObject_HasAttr(self, &_Py_ID(__notes__))) { |
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do not use PyObject_HasAttr. It is broken by design.