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

support ordering magic methods for #[pyclass] #3203

Merged
merged 1 commit into from Jun 5, 2023

Conversation

davidhewitt
Copy link
Member

@davidhewitt davidhewitt commented Jun 3, 2023

Closes #2089

This adds __lt__, __le__, __eq__, __ne__, __gt__, and __ge__ as per the Python implementations of what we call __richcmp__.

There's a UI test confirming that the user cannot implement split forms and __richcmp__ simultaneously.

There's also a benchmark comparing implementing these split methods against using __richcmp__. I couldn't see a meaningful performance difference, so I'm tempted to deprecate __richcmp__, given that's not a magic method which exists in Python. Potentially we can provide options such as the opt-in #[pyclass(eq, ord)] to avoid boilerplate for people who don't want to implement six different methods.

@davidhewitt davidhewitt force-pushed the pyclass-eq branch 2 times, most recently from c3b7a2c to a477688 Compare June 3, 2023 15:25
@davidhewitt
Copy link
Member Author

As the new behavior may break users who were attempting to use __eq__ etc unsuccessfully (unknowingly) this is a 0.20 feature.

@adamreichold
Copy link
Member

so I'm tempted to deprecate richcmp, given that's not a magic method which exists in Python. Potentially we can provide options such as the opt-in #[pyclass(eq, ord)] to avoid boilerplate for people who don't want to implement six different methods.

I think this should happen only when that boiler plate reduction is in place.

@davidhewitt
Copy link
Member Author

I think this should happen only when that boiler plate reduction is in place.

Completely agree. This PR is intended to be an addition only. I'd propose to then work on the opt-in implementation of eq and ord, and then deprecate __richcmp__ as a final step.

@davidhewitt
Copy link
Member Author

(Pushed the wording adjustment as a new commit, when we're happy with what's going on here I'll squash.)

@davidhewitt
Copy link
Member Author

Thanks, squashed!

bors r=adamreichold

bors bot added a commit that referenced this pull request Jun 4, 2023
3203: support ordering magic methods for `#[pyclass]` r=adamreichold a=davidhewitt

Closes #2089 

This adds `__lt__`, `__le__`, `__eq__`, `__ne__`, `__gt__`, and `__ge__` as per the Python implementations of what we call `__richcmp__`.

There's a UI test confirming that the user cannot implement split forms and `__richcmp__` simultaneously.

There's also a benchmark comparing implementing these split methods against using `__richcmp__`. I couldn't see a meaningful performance difference, so I'm tempted to deprecate `__richcmp__`, given that's not a magic method which exists in Python. Potentially we can provide options such as the opt-in `#[pyclass(eq, ord)]` to avoid boilerplate for people who don't want to implement six different methods.



Co-authored-by: David Hewitt <1939362+davidhewitt@users.noreply.github.com>
@bors
Copy link
Contributor

bors bot commented Jun 4, 2023

Build failed:

@adamreichold
Copy link
Member

bors retry

@bors
Copy link
Contributor

bors bot commented Jun 5, 2023

Build succeeded!

The publicly hosted instance of bors-ng is deprecated and will go away soon.

If you want to self-host your own instance, instructions are here.
For more help, visit the forum.

If you want to switch to GitHub's built-in merge queue, visit their help page.

@bors bors bot merged commit dbf7b23 into PyO3:main Jun 5, 2023
23 of 33 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.

Support __eq__ for pyclass
2 participants