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

Add as_borrowed conversion from gil-refs to Bound<T> #3692

Merged
merged 2 commits into from
Dec 26, 2023

Conversation

davidhewitt
Copy link
Member

@davidhewitt davidhewitt commented Dec 23, 2023

Update: we changed to add .as_borrowed() instead of .as_bound() as it gave additional flexibility.


This is the promised follow-up to #3686 which adds a .as_bound() method to the GIL refs API types to convert into the new Bound<T> smart pointer.

I've switched all internal code over to use this API to give a feel of how it reads; I think it's much nicer to type and read self.as_bound() over Bound::borrowed_from_gil_ref(&self). Hopefully this helps users to migrate.

I think the next PR I plan to open will be a beginning of the migration guide entry; I'd just like this conversion API agreed upon first as this will form a key part of migration instructions.

src/instance.rs Outdated Show resolved Hide resolved
src/instance.rs Outdated Show resolved Hide resolved
src/types/any.rs Outdated Show resolved Hide resolved
src/types/iterator.rs Outdated Show resolved Hide resolved
src/types/pysuper.rs Outdated Show resolved Hide resolved
@davidhewitt
Copy link
Member Author

Well, I added commits for all the suggestions here, but this PR has gotten a bit bulky as a result.

I've split some pieces and related changes into #3694, #3695, #3697 and #3698.

Let's review those separately and then I'll come back to this one to rebase it and we can review again.

@davidhewitt davidhewitt marked this pull request as draft December 24, 2023 14:59
@davidhewitt davidhewitt changed the title Add as_bound conversion from gil-refs to Bound<T> Add as_borrowed conversion from gil-refs to Bound<T> Dec 24, 2023
@davidhewitt davidhewitt force-pushed the as-bound branch 3 times, most recently from 5ef64e7 to b64c3c6 Compare December 26, 2023 09:13
@davidhewitt davidhewitt marked this pull request as ready for review December 26, 2023 09:49
@davidhewitt
Copy link
Member Author

Given that this PR is one of those PRs which conflicts with a lot of the remaining PRs which I'd like to open wrt PyXMethods trait implementations, I've come back here and dropped all the commits which are addressed in other PRs.

That means that this should now be reviewable / mergeable again and the only feedback not addressed here is the PyIterator::from_object2 change which is open to merge separately in #3702.

If this one does merge I'll rebase all the other PRs which conflict with it 🧑‍💻

@adamreichold adamreichold added this pull request to the merge queue Dec 26, 2023
Merged via the queue into PyO3:main with commit c44d2f5 Dec 26, 2023
38 checks passed
@davidhewitt davidhewitt deleted the as-bound branch December 26, 2023 12:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants