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
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
33 changes: 33 additions & 0 deletions csharp/src/Google.Protobuf.Test/WellKnownTypes/DurationTest.cs
Original file line number Diff line number Diff line change
Expand Up @@ -128,5 +128,38 @@ public void ToString_NonNormalized()
var duration = new Duration { Seconds = 1, Nanos = -1 };
Assert.AreEqual("{ \"@warning\": \"Invalid Duration\", \"seconds\": \"1\", \"nanos\": -1 }", duration.ToString());
}

[Test]
public void Comparability()
{
Duration[] durationsInExpectedSortOrder =
{
null,
new Duration { Seconds = -10, Nanos = -10 },
new Duration { Seconds = -10, Nanos = -1 },
new Duration { Seconds = -1, Nanos = -10 },
new Duration { Seconds = -1, Nanos = -1 },
new Duration(),
new Duration { Seconds = 1, Nanos = 1 },
new Duration { Seconds = 1, Nanos = 10 },
new Duration { Seconds = 10, Nanos = 1 },
new Duration { Seconds = 10, Nanos = 10 }
};

for (int i = 0; i < durationsInExpectedSortOrder.Length; i++)
{
var target = durationsInExpectedSortOrder[i];
if (target is null)
{
continue;
}
for (int j = 0; j < durationsInExpectedSortOrder.Length; j++)
{
var expectedResult = Math.Sign(i - j);
var actualResult = target.CompareTo(durationsInExpectedSortOrder[j]);
Assert.AreEqual(expectedResult, actualResult);
}
}
}
}
}
23 changes: 22 additions & 1 deletion csharp/src/Google.Protobuf/WellKnownTypes/DurationPartial.cs
Original file line number Diff line number Diff line change
Expand Up @@ -38,7 +38,7 @@ namespace Google.Protobuf.WellKnownTypes
{
// Manually-written partial class for the Duration well-known type,
// providing a conversion to TimeSpan and convenience operators.
public partial class Duration : ICustomDiagnosticMessage
public partial class Duration : ICustomDiagnosticMessage, IComparable<Duration>
{
/// <summary>
/// The number of nanoseconds in a second.
Expand Down Expand Up @@ -266,5 +266,26 @@ internal static void AppendNanoseconds(StringBuilder builder, int nanos)
}
}
}


/// <summary>
/// Given another duration, returns 0 if the durations are equivalent, -1 if this duration is shorter than the other, and 1 otherwise.
/// </summary>
/// <remarks>
/// This method expects that both durations are normalized; that is, that the values of <see cref="Seconds"/>
/// and <see cref="Nanos"/> are within the documented bounds.
/// If either value is not normalized, the results of this method are unspecified.
/// </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.

{
return other == null ? 1
: Seconds < other.Seconds ? -1
: Seconds > other.Seconds ? 1
: Nanos < other.Nanos ? -1
: Nanos > other.Nanos ? 1
: 0;
}
}
}