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

Implement PyTypeInfo for PyEllipsis, PyNone, and PyNotImplemented #3577

Merged
merged 1 commit into from Nov 25, 2023

Conversation

davidhewitt
Copy link
Member

Playing around with options to solve #3516, I noticed that we can implement PyTypeInfo without complication for the three singletons Ellipsis, None, and PyNotImplemented. So, here's the PR 😄

@davidhewitt davidhewitt force-pushed the none-typeinfo branch 2 times, most recently from c19920b to ee70af3 Compare November 15, 2023 22:56
src/types/ellipsis.rs Outdated Show resolved Hide resolved
src/types/ellipsis.rs Show resolved Hide resolved
&*ptr
#[inline]
fn is_exact_type_of(object: &PyAny) -> bool {
object.is(PyNotImplemented::get(object.py()))
Copy link
Member

Choose a reason for hiding this comment

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

I think we should add PyAny::is_not_implemented and either use these methods for all three implementations or but the three implementations here and rewrite the methods in terms of them.

Copy link
Member Author

Choose a reason for hiding this comment

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

I quite like is_none() but personally I think is_not_implemented() and is_ellipsis() are needed rarely enough that they're not worth pulling their weight.

There are also the three getter methods py.None(), py.NotImplemented() and py.Ellipsis(). Really these are just a slightly nicer way to write PyNone::get(py). (They return PyObject but really they should return &PyNone etc in my opinion, I'll open a separate PR for that in a moment.)

What do you think about deprecating obj.is_ellipsis() in favour of obj.is(py.Ellipsis())?

Copy link
Member

Choose a reason for hiding this comment

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

Works for me as well. I agree that is_none is somewhat special among the three and remove is_ellipsis is as good as adding is_not_implemented IMHO.

I would still ask for consistency here though, i.e. implement all three methods by object.is(object.py().Ellipsis()) then.

(I also don't think this needs a separate PR as getting a consistent approach to the three singletons might actually be simpler if we do it here in one PR.)

Copy link
Member Author

Choose a reason for hiding this comment

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

I decided to open #3578 specifically to change the return types. I'll come back to this PR afterwards and do the is_ellipsis() deprecation here as well as tidy up these implementations.

Copy link
Member Author

Choose a reason for hiding this comment

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

Rebased to implement all three by object.is(Self::get(py)), and also deprecate is_ellipsis.

src/types/notimplemented.rs Outdated Show resolved Hide resolved
@adamreichold adamreichold added this pull request to the merge queue Nov 25, 2023
Merged via the queue into PyO3:main with commit 81bc838 Nov 25, 2023
36 checks passed
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.

None yet

3 participants