FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

added exception groups by iritkatriel · Pull Request #25430 · python/cpython · GitHub

/ cpython Public

added exception groups - #25430

Closed
iritkatriel wants to merge 10 commits into
python:mainfrom
iritkatriel:PEP-654-ExceptionGroup
ConversationCommits (10)ChecksFiles changed
Closed

added exception groups#25430
iritkatriel wants to merge 10 commits into
python:mainfrom
iritkatriel:PEP-654-ExceptionGroup

Conversation

iritkatriel commented Apr 16, 2021 •
edited
Loading

Copy link
Copy Markdown
Member

First step of PEP-654: added Exception Groups (without updating traceback formatting).

iritkatriel requested a review from a team as a code owner April 16, 2021 00:04
iritkatriel requested review from 1st1 and gvanrossum April 16, 2021 00:05
iritkatriel commented Apr 16, 2021
View reviewed changes
Comment thread Include/pyerrors.h Outdated
Comment thread Lib/test/exception_hierarchy.txt Outdated

iritkatriel Apr 16, 2021

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Unfortunately we can't express here that ExceptionGroup subclasses BaseExceptionGroup as well.

gvanrossum Apr 16, 2021

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Add a note in parentheses?

iritkatriel Apr 16, 2021

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

There seems to be an unused feature that interprets whatever is in "(..)" as a platform name and then ignores any exception where the platform is not the current platform (so it won't break the test but will weaken it).

But "[...]" seems ok - anything in them appears to be ignored, and when I changed ExceptionGroup to something invalid the test failed.

cpython/Lib/test/test_baseexception.py

Line 49 in 8bbfeb3

if '(' in exc_name:

Comment thread PC/python3dll.c Outdated
gvanrossum reviewed Apr 16, 2021
View reviewed changes

gvanrossum left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Is this what we will merge once the PEP is accepted?

(Note that the master -> main rename will happen at that time too. But supposedly PSs will be automatically adjusted. We’ll see.)

Comment thread Include/pyerrors.h Outdated
Comment thread Lib/test/exception_hierarchy.txt Outdated

gvanrossum Apr 16, 2021

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Add a note in parentheses?

gvanrossum commented Apr 16, 2021

Copy link
Copy Markdown
Member

Probably mark as Draft?

iritkatriel marked this pull request as draft April 16, 2021 07:30
iritkatriel removed the request for review from a team April 16, 2021 09:43
iritkatriel force-pushed the PEP-654-ExceptionGroup branch from bbedd10 to 40c3cd0 Compare April 16, 2021 14:03
iritkatriel commented Apr 16, 2021
View reviewed changes

iritkatriel left a comment

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Is this what we will merge once the PEP is accepted?

That's the idea. The branch of the other PR is very old and compile.c/eval.c changed beyond what can be fixed with a rebase. So I started from scratch on a recent cut of master. Marked as draft for now.

Comment thread Lib/test/exception_hierarchy.txt Outdated

iritkatriel Apr 16, 2021

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

There seems to be an unused feature that interprets whatever is in "(..)" as a platform name and then ignores any exception where the platform is not the current platform (so it won't break the test but will weaken it).

But "[...]" seems ok - anything in them appears to be ignored, and when I changed ExceptionGroup to something invalid the test failed.

cpython/Lib/test/test_baseexception.py

Line 49 in 8bbfeb3

if '(' in exc_name:

Comment thread Include/pyerrors.h Outdated
iritkatriel force-pushed the PEP-654-ExceptionGroup branch from 81b6b08 to cecdf32 Compare April 20, 2021 18:42

gvanrossum commented May 11, 2021

Copy link
Copy Markdown
Member

You should probably merge this on main.

iritkatriel force-pushed the PEP-654-ExceptionGroup branch from cecdf32 to 0a9e6b5 Compare May 11, 2021 23:33

iritkatriel commented May 11, 2021

Copy link
Copy Markdown
Member Author

I rebased and regenerated Python/importlib_external.h

github-actions Bot commented Jun 12, 2021

Copy link
Copy Markdown

This PR is stale because it has been open for 30 days with no activity.

github-actions Bot added the stale Stale PR or inactive for long period of time. label Jun 12, 2021

iritkatriel commented Sep 26, 2021

Copy link
Copy Markdown
Member Author

Superseded by #28569.

iritkatriel deleted the PEP-654-ExceptionGroup branch October 18, 2022 14:25
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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

gvanrossum gvanrossum left review comments
1st1 Awaiting requested review from 1st1
brettcannon Awaiting requested review from brettcannon brettcannon will be requested when the pull request is marked ready for review brettcannon is a code owner
ericsnowcurrently Awaiting requested review from ericsnowcurrently ericsnowcurrently will be requested when the pull request is marked ready for review ericsnowcurrently is a code owner
ncoghlan Awaiting requested review from ncoghlan ncoghlan will be requested when the pull request is marked ready for review ncoghlan is a code owner
warsaw Awaiting requested review from warsaw warsaw will be requested when the pull request is marked ready for review warsaw is a code owner
FFY00 Awaiting requested review from FFY00 FFY00 will be requested when the pull request is marked ready for review FFY00 is a code owner

Assignees

No one assigned

Labels

awaiting review DO-NOT-MERGE stale Stale PR or inactive for long period of time.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

Back | FazBrowse Home | New Git URL