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

Allow calling Py::as_ref() and Py::into_ref() against PySequence, PyIterator and PyMapping. #1682

Merged
merged 1 commit into from
Nov 13, 2021

Conversation

moriyoshi
Copy link
Contributor

This patch allows one to call Py::as_ref() against PySequence by supplying PyTypeInfo for it.

@moriyoshi moriyoshi marked this pull request as ready for review June 17, 2021 11:38
src/ffi/abstract_.rs Outdated Show resolved Hide resolved
@moriyoshi moriyoshi force-pushed the allow-as-ref-pysequence branch from 3f821ad to b9f28dc Compare June 19, 2021 07:22
@moriyoshi moriyoshi requested a review from birkenfeld June 19, 2021 07:23
@birkenfeld birkenfeld requested review from davidhewitt and removed request for birkenfeld June 19, 2021 08:12
CHANGELOG.md Outdated Show resolved Hide resolved
@davidhewitt
Copy link
Member

Sorry for being AWOL the past week, I've been swamped and not able to keep up with OSS. This is top of my list to review either tonight or tomorrow evening.

Copy link
Member

@davidhewitt davidhewitt left a comment

Choose a reason for hiding this comment

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

Thanks, overall I'm in favour of having these two APIs for Py<PySequence>. I think it's perfectly fine to add these in manually. Maybe in the future we'll have a trait which automatically produces this API again.

If it's not too much to ask, would you be willing to add the same for Py<PyIterator>, which is in almost exactly the same position?

CHANGELOG.md Outdated Show resolved Hide resolved
src/instance.rs Outdated Show resolved Hide resolved
src/types/sequence.rs Outdated Show resolved Hide resolved
src/types/sequence.rs Outdated Show resolved Hide resolved
@moriyoshi
Copy link
Contributor Author

moriyoshi commented Jun 24, 2021

If it's not too much to ask, would you be willing to add the same for Py, which is in almost exactly the same position?

That's also what I've been wondering. Sounds nice.

@moriyoshi moriyoshi force-pushed the allow-as-ref-pysequence branch from a050004 to f115c70 Compare June 26, 2021 03:54
@moriyoshi moriyoshi changed the title Allow calling Py::as_ref() against PySequence. Allow calling Py::as_ref() and Py::into_ref() against PySequence ad PyIterator. Jun 26, 2021
@moriyoshi
Copy link
Contributor Author

I think I've done with them. Can you take a look?

Copy link
Member

@davidhewitt davidhewitt left a comment

Choose a reason for hiding this comment

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

Thanks, it's looking almost there!

There's a new implementation for IntoPyPointer for PyAny which isn't sound and needs to be removed.

Also I made a few comments to tidy up the tests a little bit.

Finally, looks like you may need to run cargo fmt before you repush.

src/types/any.rs Outdated Show resolved Hide resolved
src/types/iterator.rs Outdated Show resolved Hide resolved
src/types/iterator.rs Outdated Show resolved Hide resolved
src/types/sequence.rs Outdated Show resolved Hide resolved
src/types/sequence.rs Outdated Show resolved Hide resolved
src/types/iterator.rs Outdated Show resolved Hide resolved
src/types/iterator.rs Outdated Show resolved Hide resolved
src/types/sequence.rs Outdated Show resolved Hide resolved
@davidhewitt davidhewitt changed the title Allow calling Py::as_ref() and Py::into_ref() against PySequence ad PyIterator. Allow calling Py::as_ref() and Py::into_ref() against PySequence, PyIterator and PyMapping. Nov 12, 2021
@davidhewitt
Copy link
Member

I'm going to finish this up and also add support for PyMapping; will push onto this branch. Thanks for getting it to this point.

@davidhewitt davidhewitt force-pushed the allow-as-ref-pysequence branch 3 times, most recently from 2cfdcd9 to 00a4f43 Compare November 13, 2021 07:54
@davidhewitt davidhewitt force-pushed the allow-as-ref-pysequence branch from 00a4f43 to ae05020 Compare November 13, 2021 08:23
@davidhewitt davidhewitt merged commit c650554 into PyO3:main Nov 13, 2021
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.

4 participants