-
-
Notifications
You must be signed in to change notification settings - Fork 31.8k
gh-103791: Make contextlib.suppress also act on exceptions within an ExceptionGroup #103792
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
Conversation
ambv
commented
Apr 24, 2023
•
edited by bedevere-bot
Loading
edited by bedevere-bot
- Issue: contextlib.suppress does not support ExceptionGroups #103791
…in an ExceptionGroup
match, rest = excinst.split(self._exceptions) | ||
if rest is None: | ||
return True | ||
raise rest |
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.
This isn't ideal as it makes the exception group's own traceback include the def __exit__(...)
frame. For example:
File "/Volumes/RAMDisk/cpython/Lib/test/test_contextlib.py", line 1220, in test_exception_groups
with suppress(ValueError):
File "/Volumes/RAMDisk/cpython/Lib/contextlib.py", line 454, in __exit__
raise rest from excinst
File "/Volumes/RAMDisk/cpython/Lib/test/test_contextlib.py", line 1221, in test_exception_groups
raise eg_all()
in case of the newly added test.
This isn't a big problem because:
- the group's member exceptions contain pristine tracebacks, only the group itself contains extra frames; and
- the
raise
of the original group is still included in the traceback because we used.split()
to create the new object.
Ideally, we wouldn't need this. However, the API of __exit__
makes it impossible to replace the ExceptionGroup
instance with another one, while ExceptionGroup
itself makes its exceptions read-only.
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.
LGTM. Minor doc tweaks.
Misc/NEWS.d/next/Library/2023-04-24-23-07-56.gh-issue-103791.bBPWdS.rst
Outdated
Show resolved
Hide resolved
Co-authored-by: Irit Katriel <1055913+iritkatriel@users.noreply.github.com>
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.
(just testing out "request changes")
When you're done making the requested changes, leave the comment: |
OK, GitHub indeed doesn't disable auto-merge when changes are requested
* main: pythongh-87729: add LOAD_SUPER_ATTR instruction for faster super() (python#103497) pythongh-103791: Make contextlib.suppress also act on exceptions within an ExceptionGroup (python#103792)
return | ||
if issubclass(exctype, self._exceptions): | ||
return True | ||
if issubclass(exctype, ExceptionGroup): |
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.
Was there a specific reason to handle ExceptionGroup
but not BaseExceptionGroup
?
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.
I doubt it. Want to make a PR?