IQuantity with QuantityValue as Value type (PoC)

[lipchev]

As per our discussion over at #875 here's the proof of concept, based on the explicit struct layout proposed by @AndreasLeeb and @pgrawehr (could you please check my modification - I had to make the fields nullable in order to avoid having the DataContractSerializers spew the trash value produced by the overlapping bits).

• QuantityValue implements most of the INumber interface members / operators (operations preserving decimal when possible, failing back to double, clamped to the real values- infinities excluded) (see the QuantityValueTests and the QuantityComparisonTests)
• changed the type of IQuantity.Value/As to QuantityValue (explicit cast required)
• added back the IEquatable interface: behavior depending on the runtime type of the quantity values (strict equality comparison for double/double)
• added back the fix for the non-reflexive CompareTo issue (and the corresponding tests)
• added a check preventing the possible stack overflow exception when calling ToUnit() with default(TUnit) (or any other invalid Enum value)
• DataContracts changed to reflect the possible use of either double or decimal as ValueType (the Json converters were not changed- but probably should be)
• removed the obsolete Json converter tests

Obviously this is quite a breaking change:

1. Mass.FromKilograms(1) - the implicit conversion is to decimal instead of double, but I think is the correct behavior: here's an example where the resulting type is decimal, failing back to double as we get into the astronomical units (in contrast to this other example - where all values are doubles). I've intentionally left out a margin for the range check, as to avoid having such units loose all of their precision.
2. The bigger issue is that an explicit cast (or some ToDouble/ToDecimal method) is required- but there is no way around that.
3. The default DataContractSerializer (both json and xml) format had to change - for a WCF service that shouldn't be much of a problem, if we're breaking the interface it's guaranteed that both client and service would have to be updated (unless using a converter/data surrogate etc.). I haven't touched the json converters - the tests still pass but only because I haven't touched the IDecimalQuantity interface (which currently stands for "a quantity using decimal-based conversion functions"- not sure what we want to do about that..).
4. The IEquatable implementation seems to be correct although possibly confusing- Mass.FromKilograms(1m) == Mass.FromGrams(1000d), Mass.FromGrams(1000d) == Mass.FromGrams(1000d). Mass.FromKilograms(1d) != Mass.FromGrams(1000d).
5. Once the GenericMath becomes generally available we could think about using an INumber for the Value type

[pgrawehr]

I haven't looked at it very closely yet, but isn't this now conflicting with #1074 ?

[lipchev]

@pgrawehr Yes, quite so I'm afraid- I actually started off with the implementation from your branch, but things evolved quickly from there - up to the point where It's now a case of either or neither.
We both bring the same breaking changes to the interface- however I went one further and replaced the internal representation of the _value field as well. This prompted the need to update the DataContract which in turn made me have to change the struct layout of the QuantityValue by making the fields Nullable..

[lipchev]

Ah, damn it- I just re-read what I wrote last night, realizing that the equality contract is still broken- due to the lack of transitivity (again).. If we wanted to be strict in that regard we'd have to return false for Mass.FromKilograms(1m) == Mass.FromGrams(1000d)..

[codecov[bot]]

Codecov Report

Merging #1124 (93a7740) into release/v5 (ff307b0) will decrease coverage by 45%. The diff coverage is n/a.

@@              Coverage Diff              @@
##           release/v5   #1124      +/-   ##
=============================================
- Coverage          85%     39%     -46%     
=============================================
  Files             315     310       -5     
  Lines           48751   46593    -2158     
=============================================
- Hits            41618   18445   -23173     
- Misses           7133   28148   +21015     

Impacted Files	Coverage Δ
UnitsNet/UnitMath.cs	0% <0%> (-100%)	:arrow_down:
UnitsNet/CustomCode/Quantities/Area.extra.cs	0% <0%> (-100%)	:arrow_down:
UnitsNet/CustomCode/Quantities/Angle.extra.cs	0% <0%> (-100%)	:arrow_down:
UnitsNet/CustomCode/Quantities/Force.extra.cs	0% <0%> (-100%)	:arrow_down:
UnitsNet/CustomCode/Quantities/Energy.extra.cs	0% <0%> (-100%)	:arrow_down:
UnitsNet/CustomCode/Quantities/Volume.extra.cs	0% <0%> (-100%)	:arrow_down:
UnitsNet/CustomCode/Quantities/Duration.extra.cs	0% <0%> (-100%)	:arrow_down:
UnitsNet/CustomCode/Quantities/HeatFlux.extra.cs	0% <0%> (-100%)	:arrow_down:
UnitsNet/CustomCode/Quantities/MassFlow.extra.cs	0% <0%> (-100%)	:arrow_down:
UnitsNet/CustomCode/Quantities/MassFlux.extra.cs	0% <0%> (-100%)	:arrow_down:
... and 184 more

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

[pgrawehr]

@pgrawehr Yes, quite so I'm afraid- I actually started off with the implementation from your branch, but things evolved quickly from there - up to the point where It's now a case of either or neither.
We both bring the same breaking changes to the interface- however I went one further and replaced the internal representation of the _value field as well. This prompted the need to update the DataContract which in turn made me have to change the struct layout of the QuantityValue by making the fields Nullable..

Then, to make it easier to compare and to comply with the license, please commit your changes separately and do not squash contributions from different users to one commit.

[angularsen]

@lipchev

Ah, damn it- I just re-read what I wrote last night, realizing that the equality contract is still broken- due to the lack of transitivity (again).. If we wanted to be strict in that regard we'd have to return false for Mass.FromKilograms(1m) == Mass.FromGrams(1000d)..

I lost a bit momentum on this one after getting backlash on the change, but I personally think strict equality is the better compromise after ages of discussions. If you agree, please help me review the PR so we can get it merged.

✨ Add back IEquatable with strict equality PR 1100

Original discussion and decision for strict equality: https://github.com/angularsen/UnitsNet/issues/1017#issuecomment-1028402841

[stale[bot]]

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.