-
-
Notifications
You must be signed in to change notification settings - Fork 30.9k
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
bpo-45607: Make it possible to enrich exception displays via setting their __note__ field #29880
bpo-45607: Make it possible to enrich exception displays via setting their __note__ field #29880
Conversation
Should I add PyException_GetNote and PyException_SetNote to the c api? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice feature! LGTM. I found one path not covered by the test suite. I of course also left some pedantic PEP-7 comments 🤓
(BTW, I still find the error handling in pythonrun.c
hard to read... and it generates so many indent levels 😕)
Misc/NEWS.d/next/Core and Builtins/2021-12-01-15-38-04.bpo-45607.JhuF8b.rst
Outdated
Show resolved
Hide resolved
Co-authored-by: Erlend Egeberg Aasland <erlend.aasland@innova.no>
Co-authored-by: Erlend Egeberg Aasland <erlend.aasland@innova.no>
Thanks.
I'm working on it. I made a patch that changed so much it would take a year to review. I'll break it up. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice! LGTM, but please fix the issues Erlend brought up.
I did all of them except the test coverage for the error when passing NULL to the setter. This would only come into play if I add PyException_GetNote and PyException_SetNote to the C api. I think we need those, right? |
I think we should not add to the C API. If people want to access the note from C, they can use the |
Ah yes, very good:
|
That behavior is fine. So do you have a unit test for that? If not, it
should be easy to add one.
|
I don’t, I’ll add one. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
+1
https://bugs.python.org/issue45607