Skip to content
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

change return type of intern! macro to &Bound<PyString> #3781

Merged
merged 3 commits into from
Jan 30, 2024

Conversation

davidhewitt
Copy link
Member

Split from #3606

Primarily, this PR adds intern_bound! macro and deprecates current intern! form, to move that macro off the GIL Refs API. There are a number of locations where internal code is adjusted for this. Documentation is also updated, and in some cases reads not so nicely any more (e.g. "example: intern_bound!ing the attribute name"), but I think that I'd rather not make adjustments to the docs further in this PR, especially as we can revert those adjustments in 0.23.

This is built on top of the first commit which adds PyString::intern_bound and PyString::from_object_bound, as they were not added in #3774.

@davidhewitt davidhewitt added the CI-skip-changelog Skip checking changelog entry label Jan 30, 2024
Copy link
Member

@mejrs mejrs left a comment

Choose a reason for hiding this comment

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

Do we plan to remove intern! and rename intern_bound! to intern! eventually? If so, given it doesn't seem to break much code maybe it makes sense to just change intern! now?

@davidhewitt
Copy link
Member Author

Do we plan to remove intern! and rename intern_bound! to intern! eventually? If so, given it doesn't seem to break much code maybe it makes sense to just change intern! now?

I considered this. I think it's definitely the case that most user code will have smaller diffs by just renaming this straight away, at the cost of a few possible type errors in positions where intern! isn't directly inside of a getattr.

We could certainly start with the rename and wait for user feedback.

I'm feeling like it's worth creating a 0.21 beta release, so this would certainly be something we could correct for the final release if the feedback from the beta is that changing intern! return type is annoying.

If I don't hear any reason otherwise, maybe I'll adjust this PR to just change the return type of intern! in the next day or so and then merge this.

@alex
Copy link
Contributor

alex commented Jan 30, 2024

If pyca/cryptography is any indication, 99% of our intern!() invocations are immediately passed as an argument to call_method or getattr, so I think just changing the return type is best.

@davidhewitt
Copy link
Member Author

Ok, pushed that as a separate commit so it's easy to revert if needed later, will merge & move forward now.

@davidhewitt davidhewitt enabled auto-merge January 30, 2024 13:29
@davidhewitt davidhewitt changed the title add intern_bound! macro change return type of intern! macro to &Bound<PyString> Jan 30, 2024
@davidhewitt davidhewitt added this pull request to the merge queue Jan 30, 2024
Merged via the queue into PyO3:main with commit 6040d93 Jan 30, 2024
38 of 39 checks passed
@davidhewitt davidhewitt deleted the intern-bound branch January 30, 2024 14:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CI-skip-changelog Skip checking changelog entry
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants