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 IComparable<Duration> for the Duration type (C#) #10441

Merged
merged 1 commit into from
Aug 26, 2022

Conversation

jskeet
Copy link
Contributor

@jskeet jskeet commented Aug 23, 2022

Note that unlike Timestamp, this does not also overload comparison
operators. Adding a == overload now would be a breaking change (as
it would change the meaning of existing code from a reference
comparison to a value comparison), and implementing <, <=, >=
and > without implementing == would be odd.

Implementing IComparable<T> makes sorting much easier, however.

Fixes #7628

Note that unlike Timestamp, this does *not* also overload comparison
operators. Adding a `==` overload now would be a breaking change (as
it would change the meaning of existing code from a reference
comparison to a value comparison), and implementing `<`, `<=`, `>=`
and `>` without implementing `==` would be odd.

Implementing `IComparable<T>` makes sorting much easier, however.

Fixes protocolbuffers#7628
Copy link

@amanda-tarafa amanda-tarafa left a comment

Choose a reason for hiding this comment

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

LGTM with a note.

/// </remarks>
/// <param name="other">The duration to compare with this object.</param>
/// <returns>An integer indicating whether this duration is shorter or longer than <paramref name="other"/>.</returns>
public int CompareTo(Duration other)

Choose a reason for hiding this comment

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

Unknown fields are not being taken into account here but they are being taken into account for equality and hashcode. Not sure how to take them into account but it may happen that something equals here is then not equals with Equals.

Maybe a note in the docs is enough.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Given that this is a well-known type, we really shouldn't be getting any unknown fields - I think it's fine for the comparison to ignore them.

@jskeet jskeet assigned fowles and unassigned amanda-tarafa Aug 26, 2022
@jskeet
Copy link
Contributor Author

jskeet commented Aug 26, 2022

@fowles: If you could merge this, that would be great :)

@fowles fowles merged commit ad42a97 into protocolbuffers:main Aug 26, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

WellKnownTypes.Duration does not implement IComparable
4 participants