IComparable is for sort order not if two objects are equals

[liborweigl]

Hi All,

I think remove new ComparableComparer<T>() from Comparers strategy should fix this bug.

machine/machine.specifications#372

Thanks

Libor

[robertcoltheart]

I don't think this is right, as we still want to retain the ability to calculate equality using IComparable.

I have no idea why you would have different definitions of 'equality' for IEquatable and IComparable (see your example in machine/machine.specifications#372), but if you really needed this to be the case, I would change the order and put the ComparableComparer below the EquatableComparer, so that your use case passes.

Also this needs unit tests.

[ulrichb]

@robertcoltheart In the following issue I argued why prefering IComparable over IEquatable is not a good idea (In short: The IComparable contract is not about (value) equality).

See fluentassertions/fluentassertions#172.

[robertcoltheart]

We're in the same boat as FluentAssertions, which is that we would break any existing uses of this package. I stand by the statement above, which is that it's preferable to check for IEquatable and then check IComparable for equality. This checking is reversed at present, which is probably wrong.

For reference, since we're talking about how different packages deal with this, xunit also checks IComparable after IEquatable (see here).

[liborweigl]

Hi All,

Yes, I fully agree with @ulrichb over IComparable vs IEquatable. And I also came up with same idea change order in compare strategy. But my conclusion is that we shouldn't use IComparable for equality.

I can add tests coverage if we decide go head with this change.

Thanks

Libor

[robertcoltheart]

I'm happy to make the change, but, similar to the way xunit does it, I'd want to check IEquatable and then check IComparable in that order. Reordering the comparers in the factory will achieve that.

[liborweigl]

Yes, please swap the order. IEquatable should be before IComparable in compare strategy. It will partly fix this issue but I still think that IComparable shouldn't be there at all.