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 CompareOp::matches #2460

Merged
merged 1 commit into from Jun 21, 2022
Merged

add CompareOp::matches #2460

merged 1 commit into from Jun 21, 2022

Conversation

birkenfeld
Copy link
Member

to easily implement __richcmp__ as the result of a Rust std::cmp::Ordering comparison.

@birkenfeld birkenfeld force-pushed the ordering_helper branch 3 times, most recently from c890890 to ee190ab Compare June 17, 2022 09:33
@birkenfeld
Copy link
Member Author

Questions:

  • is this name obvious enough?
  • should it take Option<Ordering> to allow for partial_cmp results?

@CLOVIS-AI
Copy link
Contributor

What about op.from_ordering(…) instead of op.convert(…)?

I don't think convert is clear enough on what it means.

@birkenfeld
Copy link
Member Author

from_ methods normally construct an instance of their type, which this doesn't.

@mejrs
Copy link
Member

mejrs commented Jun 20, 2022

  • is this name obvious enough?

What about matches? Or could this be a PartialEq impl?

@birkenfeld
Copy link
Member Author

matches works for me! Not sure about PartialEq, it might be confusing that Ordering::Less == CompareOp::Le etc.

to easily implement `__richcmp__` as the result of a Rust `std::cmp::Ordering` comparison.
@birkenfeld birkenfeld changed the title add CompareOp::convert add CompareOp::matches Jun 21, 2022
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.

LGTM

@birkenfeld
Copy link
Member Author

Thanks for the review!

@birkenfeld birkenfeld merged commit 53b83cc into main Jun 21, 2022
@birkenfeld birkenfeld deleted the ordering_helper branch June 21, 2022 13:36
@davidhewitt
Copy link
Member

👍 good idea, thanks for this & sorry I've been slow on reviews recently!

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

4 participants