-
-
Notifications
You must be signed in to change notification settings - Fork 1.6k
PEP 785: New methods for easier handling of ExceptionGroup
s
#4357
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
This comment was marked as resolved.
This comment was marked as resolved.
2645fb7
to
6656d1f
Compare
Why not use the next available number (785)? |
Because I've also got PEP-789, and I was worried about getting them mixed up in conversation 😅 |
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.
Overall looks good, left some feedback inline.
From PEP 1:
I don't have strong feelings, but I think we should encourage authors to keep PEP numbers in order, we've had recent confusion with gaps etc in the assigned numbers (777, 789). A |
ExceptionGroup
s
I agree, we don't want authors jumping to the next tens if they already have a PEP in the current one, it'll be tricky to defrag (and we don't want to run out of integers 🙃). |
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.
Initial brief review:
This has again caused confusion, see #4365, where the author chose PEP 791 as the 'next' number. @Zac-HD please could you renumber this PEP to 785, and then we can assign #4365 as PEP 786 (assuming it gets a sponsor)? A |
ExceptionGroup
sExceptionGroup
s
125cb75
to
c6f30fc
Compare
Thank you all for the reviews - I've fixed the PEP number, and also implemented the suggestions from first-pass review. (even deleting the second space after each period!) Looking at the build failure, I don't think that's me - it hasn't even read that far yet:
|
Co-authored-by: Hugo van Kemenade <[email protected]>
Ready to merge? |
Did a final self-review and fixed some formatting and minor expression issues; I'm now ready to merge. Thanks again to everyone above! |
we believe all comments were addressed and github just isn't gettin' it.
I used Claude Code to find the comments on the PR that GitHub refused to expose in its UI and make Github GraphQL API calls to mark them as resolved. there were two stranded review comments unresolved from hugo 5 days ago that the force push'es had garbage collected from git so the github UI wouldn't allow you to see them from the files changes -> conversations view other than linking to broken links to them. |
(if you're a |
@Zac-HD Please could you open a quick PR to add the discussion thread to |
I've discussed this fairly extensively with @gpshead, and we've been using related functions at work for some time now. I'm therefore posting the PR now to get the process rolling; I also expect to polish the prose (especially the footnotes) a little further before merging.
Basic requirements (all PEP Types)
pep-NNNN.rst
), PR title (PEP 123: <Title of PEP>
) andPEP
header(apologies for the intitial confusion here)
Author
orSponsor
, and formally confirmed their approvalAuthor
,Status
(Draft
),Type
andCreated
headers filled out correctlyPEP-Delegate
,Topic
,Requires
andReplaces
headers completed if appropriate.github/CODEOWNERS
for the PEPStandards Track requirements
(none identified)
(none identified)
Python-Version
set to valid (pre-beta) future Python version, if relevantDiscussions-To
andPost-History
📚 Documentation preview 📚: https://pep-previews--4357.org.readthedocs.build/