Separate comparable types

[dbuckin1]

This implements the change from discussion 551.

• ValueObject and SimpleValueObject no longer implement IComparable or IComparable<T>
• Introduces new types ComparableValueObject and SimpleComparableValueObject with the same comparison behavior that was removed from ValueObject and SimpleValueObject
• Added tests for the new types to match the existing tests

Additionally, it is worth noting that

• I did not change ValueObject<T>. It already did not implement IComparable
• I left the comparison behavior on the Entity and EnumValueObject types as it seems unlikely that use of IComparable would be particularly cumbersome for identifiers
• This change undoes the breaking behavior from version 2.38.0 (see section 4 of the discussion) but is breaking in its own right with respect to the current comparison behavior

[vkhorikov]

Sorry for the late reply here. I have a bit more time now, so will reply faster now. Apologies again.

Do you think we need ComparableSimpleValueObject? I'd like to avoid combinatorial explosion with these types if possible. Everything else looks good to me.

cc @hankovich

[bothzoli]

I think I'd call it SimpleComparableValueObject rather than ComparableSimpleValueObject.

And I think the reason why ComparableSimpleValueObject would be needed is that it'd save library users who use a single value value object a lot of boilerplate code. See ComparableEmailAddress in CSharpFunctionalExtensions.Tests/ValueObjectTests/BasicTests.cs.

public class EmailAddress : SimpleValueObject<string>
{
    public EmailAddress(string value) : base(value) { }
}

On it's own I don't think ComparableValueObject makes much sense (as why would you have a comparable something if it doesn't have anything to compare within), however it does make a lot of sense if you'd like to use it with multiple values, like ComparableMoney in CSharpFunctionalExtensions.Tests/ValueObjectTests/BasicTests.cs. If you didn't have this, you'd need to create a Money : IComparable class and use that as ComparableSimpleValueObject<Money> to get the same result.

public class ComparableMoney : ComparableValueObject
{
    public string Currency { get; }
    public decimal Amount { get; }

    public ComparableMoney(string currency, decimal amount)
    {
        Currency = currency;
        Amount = amount;
    }

    protected override IEnumerable<IComparable> GetComparableEqualityComponents()
    {
        yield return Currency.ToUpper();
        yield return Math.Round(Amount, 2);
    }
}

So, while the two classes could be combined (either by providing a default implementation to GetComparableEqualityComponents to return an empty enumerable, or adding a single T value to ComparableValueObject) I think both of those would impose unnecessary restrictions on library users.

[dbuckin1]

I originally wanted to remove SimpleValueObject's requirement that its Value property implement IComparable, as I figured this requirement would be cumbersome to the user in the same manner as ValueObject's analogous requirement.

However, thinking about it more, I'm not sure that there is a use case for SimpleValueObject over ValueObject except to save some code when wrapping a single primitive. (I suppose this probably is the intended purpose of SimpleValueObject?) Given that the only primitive that does not implement IComparable is bool (is there any reason to wrap a bool in SimpleValueObject over using an enumeration type or something?), I actually do think it would be fine to just leave the requirement on SimpleValueObject.

@bothzoli, this would retain the existing behavior of saving the user boilerplate. So @vkhorikov, if you are good with that, I will update the PR to revert to the prior SimpleValueObject and remove ComparableSimpleValueObject.

[vkhorikov]

except to save some code when wrapping a single primitive

Yes, this is exactly the intended purpose of it.

So, we're removing SimpleValueObject and renaming ComparableSimpleValueObject into SimpleValueObject, correct? If so, let's go ahead with this, I think it's a good idea. It also addresses the concerns @bothzoli raised (@bothzoli chime in if you disagree).

[bothzoli]

Yes, it makes perfect sense.

[dbuckin1]

Sounds good. I have restored the comparable behavior of SimpleValueObject and removed ComparableSimpleValueObject.

[vkhorikov]

Great! Merged.

[zakuk]

@vkhorikov, do you know when the next NuGet release will be that includes this merge? :)

[vkhorikov]

This is going to be released as v3.0.0 because of the breaking changes. I wanted to include another breaking change in the release, so most likely will release the new version this coming weekend.

[vkhorikov]

Pushed v3 to NuGet.