ValueObject : IComparable<ValueObject> is Potentially Breaking due to Type Ambiguity

[SirBuckinghamVI]

After updating to version 2.12.0, many of my tests are breaking due to the addition of IComparable to ValueObject and the corresponding requirement that the elements of GetEqualityComponents implement IComparable, as the addition of the new interface introduces type ambiguity.

Specifically, I am running into this issue with FluentAssertions. Previously, when calling Should() from FluentAssertions, the extension method of a general object was used, but now that ValueObject implements IComparable, it chooses the extension method of IComparable. The tests then fail when calling the Be() method from FluentAssertions, because it checks that the comparison returns 0, but my ValueObjects include types in GetEqualityComponents that do not implement IComparable.

To this end, is it not an ISP violation to make ValueObject implement IComparable? There is no guarantee that a comparison operation makes sense for a ValueObject. For example, I have a geocoordinate ValueObject, but it doesn't really make sense to say that one point on the globe is greater or less than another. Would it not make more sense to have clients implement IComparable in their deriving classes?

[SirBuckinghamVI]

Changing the CompareTo implementation to just skip components that don't implement IComparable would not address my ISP concern, but it would patch the overload ambiguity issue in cases like mine. I can make a PR if that is suitable to you.

[vkhorikov]

IComparable might need to be introduced in another base class (ComparableValueObject instead of ValueObject) to avoid these issues.

IComparable is only needed for sorting/ordering, though, its implementation doesn't have to have the semantics of "greater than" or "less than", just like with "A" and "B". In your example with geocoordinates, you should have the IComparable implementation by default if the value objects consists of primitive types.

Also, in your other example, what is the member of the value object that doesn't implement IComparable?

[SirBuckinghamVI]

That's a good point. The main member that does not implement IComparable that I have used is NodaTime.Interval. I imagine it does not implement it because of the ambiguity of which characteristic of the interval it should sort by. Others include types for representing location such as postal codes, and value objects representing an interval of work for an employee to do at a given time and location.

[vkhorikov]

I'll extract the IComparable implementation into a a separate base class. Will try to do that today.

[vkhorikov]

Decided to default the CompareTo result to 0 instead of introducing a new base class. Let me know if it fixes your issue.

[SirBuckinghamVI]

It's close, but now it is failing on NotBe instead of Be, since in the current stable version of FluentAssertions, NotBe for IComparable expects CompareTo to not return zero. It does look like in the next major release of FluentAssertions, NotBe will only look at equality, not comparisons. What are your thoughts here?

[SirBuckinghamVI]

Eric Lippert says that "ideally", equality and comparison methods should be consistent. But not a rule, it seems.

[vkhorikov]

Thanks for the link, Eric is a very good explainer. I've pushed an update -- this time if a component doesn't implement IComparable, the base class returns 0 only if the two components are equal. Let me know if this fixes your issue.

[SirBuckinghamVI]

That did it! Thanks!