GV

GeezerVison

The World Wide Web, restored to 800 pixels of glory.

Home|About|Gaming cabinet|Archives|How it works|Starter sites

On this site

  • Sign in
  • ActionsCI/CD and automation
  • CodespacesCloud development environments
  • All Features
  • Documentation
  • Training
  • Certifications
  • Build with Copilot
  • Customize Copilot
  • Integrations
  • Trending repos
  • GitHub Sponsors
  • Developer articles
  • Enterprises

This view

  • Original page
  • New address

GeezerVison is showing github.com — gh-99553: add tests for ExceptionGroup wrapping by Zac-HD · Pull Request #99615 · python/cpython

Conversation

@Zac-HD

@Zac-HD Zac-HD commented Nov 20, 2022 •
edited by bedevere-bot
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Contributor

Follows #99572 (comment) to clarify a potential point of confusion. CC @iritkatriel (thank you again!)

  • Issue: Subclasses of ExceptionGroup can wrap BaseExceptions #99553

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
@Zac-HD
Zac-HD requested a review from iritkatriel as a code owner November 20, 2022 08:13
@bedevere-bot bedevere-bot added awaiting review tests Tests in the Lib/test dir labels Nov 20, 2022
@Zac-HD
pythongh-99553: clarify doc and add tests
4b9e01a
@Zac-HD
Zac-HD force-pushed the zac/custom-exceptiongroup-docs-tests branch from 8158778 to 4b9e01a Compare November 20, 2022 08:26
@sobolevn sobolevn added the skip news label Nov 20, 2022
@sobolevn

sobolevn commented Nov 20, 2022

Copy link
Copy Markdown
Member

@Zac-HD I cannot do that, but if you change you PR title from gh-99572 to gh-99553 - the CI failure will go away :)
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@AlexWaygood AlexWaygood changed the title gh-99572: clarify doc and add tests gh-99553: clarify doc and add tests Nov 20, 2022
@bedevere-bot bedevere-bot mentioned this pull request Nov 20, 2022
Subclasses of ExceptionGroup can wrap BaseExceptions #99553
Closed
@hauntsaninja hauntsaninja closed this Nov 20, 2022
@hauntsaninja hauntsaninja reopened this Nov 20, 2022
hauntsaninja
hauntsaninja reviewed Nov 20, 2022
View reviewed changes
Comment thread Doc/library/exceptions.rst Resolved

Uh oh!

There was an error while loading. Please reload this page.
@Zac-HD @hauntsaninja
Update Doc/library/exceptions.rst
29b9f27 
Co-authored-by: Shantanu <[email protected]>
iritkatriel
iritkatriel reviewed Nov 20, 2022
View reviewed changes
Comment thread 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`.

@iritkatriel iritkatriel Nov 20, 2022

Copy link
Copy Markdown
Member

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@Zac-HD Zac-HD Nov 20, 2022

Copy link
Copy Markdown
Contributor Author

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@iritkatriel iritkatriel Nov 20, 2022

Copy link
Copy Markdown
Member

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
@AlexWaygood AlexWaygood changed the title gh-99553: clarify doc and add tests gh-99553: ExceptionGroups: clarify doc and add tests Nov 20, 2022
hauntsaninja
hauntsaninja reviewed Nov 21, 2022
View reviewed changes
Comment thread Doc/library/exceptions.rst Resolved

Uh oh!

There was an error while loading. Please reload this page.
@Zac-HD @hauntsaninja
Update Doc/library/exceptions.rst
07bdfea 
Co-authored-by: Shantanu <[email protected]>
@Zac-HD

Zac-HD commented Nov 29, 2022

Copy link
Copy Markdown
Contributor Author

Ready to merge, I think?
hauntsaninja reacted with thumbs up emoji
All reactions
  • 👍 1 reaction

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
iritkatriel
iritkatriel reviewed Nov 29, 2022
View reviewed changes
Comment thread Doc/library/exceptions.rst Outdated
Comment on lines +968 to +971
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`.

@iritkatriel iritkatriel Nov 29, 2022

Copy link
Copy Markdown
Member

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?

Suggested change
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`.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
kumaraditya303
kumaraditya303 requested changes Jan 1, 2023
View reviewed changes

@kumaraditya303 kumaraditya303 left a comment

Copy link
Copy Markdown
Contributor

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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
@bedevere-bot bedevere-bot removed the awaiting review label Jan 1, 2023
@bedevere-bot

bedevere-bot commented Jan 1, 2023

Copy link
Copy Markdown

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 have made the requested changes; please review again. I will then notify any core developers who have left a review that you're ready for them to take another look at this pull request.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@bedevere-bot bedevere-bot added the awaiting changes label Jan 1, 2023
@Zac-HD

Zac-HD commented Apr 11, 2023

Copy link
Copy Markdown
Contributor Author

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 🙂
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@Zac-HD Zac-HD closed this Apr 11, 2023
@hauntsaninja

hauntsaninja commented Apr 11, 2023 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Contributor

If it's hard to figure out appropriate changes to the docs, I can at least merge the tests.
Zac-HD reacted with thumbs up emoji
All reactions
  • 👍 1 reaction

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@hauntsaninja hauntsaninja reopened this Apr 11, 2023
@hauntsaninja
undo docs changes
9bdb9c6
@hauntsaninja hauntsaninja added the needs backport to 3.11 only security fixes label Apr 11, 2023
@hauntsaninja
Merge branch 'main' into zac/custom-exceptiongroup-docs-tests
774b0b8
@hauntsaninja
Merge branch 'main' into zac/custom-exceptiongroup-docs-tests
043d002
@hauntsaninja hauntsaninja changed the title gh-99553: ExceptionGroups: clarify doc and add tests gh-99553: add tests for ExceptionGroup wrapping Apr 11, 2023
@hauntsaninja
hauntsaninja merged commit 4cd1cc8 into python:main Apr 11, 2023
@miss-islington

miss-islington commented Apr 11, 2023

Copy link
Copy Markdown
Contributor

Thanks @Zac-HD for the PR, and @hauntsaninja for merging it 🌮🎉.. I'm working now to backport this PR to: 3.11.
🐍🍒⛏🤖
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@bedevere-bot bedevere-bot removed the awaiting changes label Apr 11, 2023
miss-islington pushed a commit to miss-islington/cpython that referenced this pull request Apr 11, 2023
@Zac-HD @miss-islington
pythongh-99553: add tests for ExceptionGroup wrapping (pythonGH-99615)
0fd85aa 
(cherry picked from commit 4cd1cc8)

Co-authored-by: Zac Hatfield-Dodds <[email protected]>
@bedevere-bot

bedevere-bot commented Apr 11, 2023

Copy link
Copy Markdown

GH-103435 is a backport of this pull request to the 3.11 branch.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@bedevere-bot bedevere-bot removed the needs backport to 3.11 only security fixes label Apr 11, 2023
@hauntsaninja

hauntsaninja commented Apr 11, 2023

Copy link
Copy Markdown
Contributor

Thank you!
Zac-HD reacted with heart emoji
All reactions
  • ❤️ 1 reaction

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
miss-islington added a commit that referenced this pull request Apr 11, 2023
@miss-islington @Zac-HD
gh-99553: add tests for ExceptionGroup wrapping (GH-99615)
254494c 
(cherry picked from commit 4cd1cc8)

Co-authored-by: Zac Hatfield-Dodds <[email protected]>
@Zac-HD
Zac-HD deleted the zac/custom-exceptiongroup-docs-tests branch April 11, 2023 07:58
warsaw pushed a commit to warsaw/cpython that referenced this pull request Apr 11, 2023
@Zac-HD @warsaw
pythongh-99553: add tests for ExceptionGroup wrapping (python#99615)
18df988
aisk pushed a commit to aisk/cpython that referenced this pull request Apr 18, 2023
@Zac-HD @aisk
pythongh-99553: add tests for ExceptionGroup wrapping (python#99615)
dd9946d
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@iritkatriel iritkatriel iritkatriel left review comments
@hauntsaninja hauntsaninja hauntsaninja left review comments
@kumaraditya303 kumaraditya303 kumaraditya303 requested changes

Uh oh!

There was an error while loading. Please reload this page.

Assignees

No one assigned

Labels

skip news tests Tests in the Lib/test dir

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Uh oh!

There was an error while loading. Please reload this page.

7 participants

@Zac-HD @sobolevn @bedevere-bot @hauntsaninja @miss-islington @iritkatriel @kumaraditya303
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
You are visitor 01402503 — GeezerVison does not store the pages you look up.
Best viewed with Netscape Navigator 4.7 or Internet Explorer 6 at 800×600. No cookies required.