Skip to content

PEP 788: Add some minor clarifications #4391

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 9 commits into from
Apr 30, 2025
Merged

PEP 788: Add some minor clarifications #4391

merged 9 commits into from
Apr 30, 2025

Conversation

ZeroIntensity
Copy link
Member

@ZeroIntensity ZeroIntensity commented Apr 27, 2025 •
edited
Loading

This doesn't change anything about the PEP, but fixes a few clarity issues that quickly came up. Sorry for the churn and extra PR here :(

📚 Documentation preview 📚: https://pep-previews--4391.org.readthedocs.build/pep-0788/

@ZeroIntensity ZeroIntensity requested a review from vstinner as a code owner April 27, 2025 21:33
@ZeroIntensity
Copy link
Member Author

ZeroIntensity commented Apr 27, 2025 •
edited
Loading

While we're here, please let me know if there are any other unclear sections. I have a bit of expert blindness, and I'd really rather make this the only follow-up for English issues.

@AA-Turner
Copy link
Member

I'd really rather make this the only follow-up for English issues.

There's no problem at all with more follow-ups, we'd rather have a clearer PEP than any arbitary limit on the number of edits allowed!

A

vstinner
vstinner reviewed Apr 28, 2025
View reviewed changes
@ZeroIntensity @vstinner
Update pep-0788.rst
81a71a1 
Co-authored-by: Victor Stinner <vstinner@python.org>
vstinner
vstinner reviewed Apr 28, 2025
View reviewed changes
Copy link
Member

@vstinner vstinner left a comment

Choose a reason for hiding this comment

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

In the case where it is useful to let the interpreter finalize, such as in a signal handler where there’s no guarantee that the thread will start, strong references to an interpreter can be acquired through PyInterpreterState_Lookup().

I suggest to remove mention of "signal handler". IMO a signal handler should not and cannot use the Python C API. There are too many constraints to write async-signal safe code.

@vstinner
Copy link
Member

You may add a PyThreadState_GetDaemon() function.

@ZeroIntensity
Copy link
Member Author

I have a few concerns about PyThreadState_GetDaemon. Let's do that in a different PR where I can update the rationale and specification, because we'll likely need to change how threading finalizes its threads for that to be practical.

@ZeroIntensity
Copy link
Member Author

@AA-Turner @hugovk Could one of you give this a quick review? This is supposed to just add a few clarifications, not change the proposal. I'll do a big "round 1 comments" PR separately.

@CAM-Gerlach CAM-Gerlach requested a review from AA-Turner April 30, 2025 09:35
CAM-Gerlach
CAM-Gerlach reviewed Apr 30, 2025
View reviewed changes
Copy link
Member

@CAM-Gerlach CAM-Gerlach left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Not Adam or Hugo so I'm coming in blind here (though perhaps not the worst thing from the "curse of knowledge" perspective you refer to), but I gave it a quick PEP Editor review for ya. Looked good aside from a couple small clarity suggestions!

@CAM-Gerlach CAM-Gerlach requested a review from vstinner April 30, 2025 09:53
@ZeroIntensity @CAM-Gerlach
Apply suggestions from code review
d553a89 
Co-authored-by: C.A.M. Gerlach <CAM.Gerlach@Gerlach.CAM>
@AA-Turner AA-Turner merged commit b0cc506 into python:main Apr 30, 2025
5 checks passed
@ZeroIntensity ZeroIntensity deleted the pep-788-clarity branch April 30, 2025 17:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@CAM-Gerlach CAM-Gerlach CAM-Gerlach left review comments

@AA-Turner AA-Turner AA-Turner approved these changes

@pitrou pitrou Awaiting requested review from pitrou

@vstinner vstinner Awaiting requested review from vstinner vstinner is a code owner

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@ZeroIntensity @AA-Turner @vstinner @CAM-Gerlach