-
-
Notifications
You must be signed in to change notification settings - Fork 32k
gh-99553: add tests for ExceptionGroup wrapping #99615
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
gh-99553: add tests for ExceptionGroup wrapping #99615
Conversation
8158778
to
4b9e01a
Compare
@Zac-HD I cannot do that, but if you change you PR title from |
Co-authored-by: Shantanu <[email protected]>
Doc/library/exceptions.rst
Outdated
A subclass of :exc:`BaseExceptionGroup` that extends :exc:`Exception` | ||
cannot wrap :exc:`BaseException`\ s, as is the case with :exc:`ExceptionGroup`. | ||
However, there is no further restriction: a :exc:`BaseExceptionGroup` subclass | ||
which extends :exc:`KeyError` could wrap any :exc:`Exception`. |
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 think this is a strange thing to say in a doc, because why would you expect it to not wrap any Exception?
Maybe instead of the second sentence we could just explain the motivation for the restriction: “This ensures that wrapped BaseExceptions are not caught by ‘except Exception’.”
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.
why would you expect it to not wrap any Exception?
Suppose we have class MyEG(BaseExceptionGroup, X):
, where except X:
will catch such a group because it extends X
. We disallow wrapping BaseException
s in an ExceptionGroup
because they would not be caught by except Exception:
if unwrapped.
However this logic is not applied consistently: if X
is e.g. KeyError
, then except KeyError:
can indeed catch MyEG("", [ValueError()])
. I find this inconsistency surprising, which is why I commented on #99572 and then opened this PR!
(I'm not trying to reopen the design discussion, just trying to explain my perspective)
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.
Ok, I see. Well yes, the Exception case is different - people use ‘expect Exception’ as ‘catch everything except fatal errors’.
Co-authored-by: Shantanu <[email protected]>
Ready to merge, I think? |
Doc/library/exceptions.rst
Outdated
A subclass of :exc:`BaseExceptionGroup` that extends :exc:`Exception` | ||
cannot wrap :exc:`BaseException`\ s, as is the case with :exc:`ExceptionGroup`. | ||
However, there is no further restriction: a :exc:`BaseExceptionGroup` subclass | ||
which extends :exc:`KeyError` could wrap any :exc:`Exception`. |
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 don't think this comment is easy to understand without the justification. How about this?
A subclass of :exc:`BaseExceptionGroup` that extends :exc:`Exception` | |
cannot wrap :exc:`BaseException`\ s, as is the case with :exc:`ExceptionGroup`. | |
However, there is no further restriction: a :exc:`BaseExceptionGroup` subclass | |
which extends :exc:`KeyError` could wrap any :exc:`Exception`. | |
Like :exc:`ExceptionGroup`, user-defined subclasses of :exc:`BaseExceptionGroup` | |
that extend :exc:`Exception` can only wrap :exc:`Exception` subclasses. | |
This is to prevent exceptions that represent fatal errors from being wrapped in an | |
exception group which can be caught by ``except Exception``. See also :ref:`exceptions`. |
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.
Please address Irit's review.
A Python core developer has requested some changes be made to your pull request before we can consider merging it. If you could please address their requests along with any other requests in other reviews from core developers that would be appreciated. Once you have made the requested changes, please leave a comment on this pull request containing the phrase |
I've taken a few attempts, and haven't found a way to explain this which reliably clarifies rather than confuses, so I'll just close the PR 🙂 |
If it's hard to figure out appropriate changes to the docs, I can at least merge the tests. |
Thanks @Zac-HD for the PR, and @hauntsaninja for merging it 🌮🎉.. I'm working now to backport this PR to: 3.11. |
(cherry picked from commit 4cd1cc8) Co-authored-by: Zac Hatfield-Dodds <[email protected]>
GH-103435 is a backport of this pull request to the 3.11 branch. |
Thank you! |
(cherry picked from commit 4cd1cc8) Co-authored-by: Zac Hatfield-Dodds <[email protected]>
Follows #99572 (comment) to clarify a potential point of confusion. CC @iritkatriel (thank you again!)
ExceptionGroup
can wrapBaseException
s #99553