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 PartialOrd on Duration #1162

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Conversation

wcarmon
Copy link

@wcarmon wcarmon commented Sep 18, 2024

allow duration comparisons (similar to chrono::TimeDelta, which has the same structure)

https://docs.rs/chrono/latest/src/chrono/time_delta.rs.html#52

allow duration comparisons (similar to chrono::TimeDelta, which has the same structure)

https://docs.rs/chrono/latest/src/chrono/time_delta.rs.html#52
Copy link
Collaborator

@caspermeijn caspermeijn left a comment

Choose a reason for hiding this comment

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

I like the idea.

  1. protobuf.rs is a generated file. When the tests are run, it will be overwritten. This could be fixed by moving the derive to here:
    .btree_map(["."])

    Then running the tests will generate a new version with your derive.
  2. Duration is not guaranteed to be normalized. That means that nanos field could be larger than 1 second and the ordering would be off. I am unsure how big this problem is, as the documentation states that it should be within range. Let's ignore normalization for now.
  3. I would like to see some tests to confirm the ordering works as expected. As I understand the documentation of derive(Ord) it sorts based on the alphabetical order, which would be incorrect in this case: https://dev-doc.rust-lang.org/stable/std/cmp/trait.PartialOrd.html#derivable
  4. Are you aware of this earlier work? Maybe you could take that as inspiration: Feat: Implements ord and partialord to timestamp #995

@caspermeijn caspermeijn changed the title Update protobuf.rs Implement PartialOrd on Duration Sep 20, 2024
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