Correct support for quantities with decimal base type
Fixes #1058
Proposal for fixing #1058. As per discussion:
-
IQuantity.ValuereturnsQuantityValue. This implies the addition of some explicit interface implementations to the quantities, so that the defaultValueproperty can stay double/decimal. - The conversion properties return double or decimal, depending on the quantity base type.
Some trickery is now required to be able to compare instances of QuantityValue with different base types. I also added a ToQuantity method (maybe not the best name) to simplify converting an Quantity to a QuantityValue. This is now required when multiplying values with different base types (because there's not predefined operator to multiply or divide double and decimal).
This is considered a draft, the unit tests have not been updated yet.
Will get to this, just a bit short on time these days.
@angularsen No worries, take your time. Good coders are always busy ;-)
I was not able to push a change, so here is the diff I tried to push regarding As() overloads:
0001-Simplify-As-overloads-by-reusing-and-casting-to-doub.patch.txt
@angularsen There seems to be a weird problem with the use of UnitsNetJsonConverter that now throws a MissingMethodException I don't fully understand. Possibly due to a mix of using the interface (that has an explicit implementation of Value now) and the reflection that happens inside the json writer. But since that class is marked obsolete anyway, I'm not sure it's worth fixing it. Can UnitsNetJsonConverter be dumped?
On the phone, but we can't make breaking changes in v4. Either we must fix it or move this change to v5 branch.
@angularsen The entire PR is intended to go to v5 only.
Aha, my bad.
UnitsNetJsonConverter seems already removed in v5 branch, but it seems the backwards compatibility tests are failing on it. I find that odd, I thought those tests solely dependend on v4 nugets in order to test backwards compatibility but maybe they do bring v5 implementation into the mix.
I think we need to figure out why these tests are failing and fix them, because I don't want to remove the compatibility tests so we can make sure we aren't breaking anything.
https://ci.appveyor.com/project/angularsen/unitsnet/builds/43692947/tests
I don't have time to help look into it right now, but might be able to assist later this week.
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.
Hey guys, I've just had a look at this - and it appears that the only tests that are failing are from the UnitsNet.Serialization.JsonNet.CompatibilityTests project- which IMO should have been removed altogether in v5. It only tests the UnitsNetJsonConverter (the converter is already removed in v5) - via the official nuget!? Here's an extract from csproj:
<!--Get the latest released version of UnitsNet.Serialization.JsonNet in Nuget-->
<PackageReference Include="UnitsNet.Serialization.JsonNet" Version="4.4.0" />
@lipchev Yea, it's still strange that the test fails, as it only uses old implementations (or maybe not?) I forgot about this, but I'll have a look when I find time.
It's incorrect in referencing the converter via the nuget, while the UnitsNet core is referenced using a project reference. Had the reference to the converter been via direct reference it would have caused a compile time issue at the moment that the old converter was removed, thus forcing us to also remove the converter tests (i.e. the whole project)..
@angularsen You write above that you want the compatibility tests still to pass, but given v5 doesn't even have the UnitsNetJsonConverter, I agree that the tests should just be removed. There's nothing we can compare compatibility with if the new implementation neither reads nor writes data in this format.
Although it might be argued that the tests actually did their intended purpose- if that was to test the latest UnitsNet version against UnitsNet.Serialization.JsonNet v4.4.0 - it was correct in establishing that UnitsNet v5 is not compatible with it. :)
I just removed the compatibility tests from release/v5 and merged it into here, I agree they no longer serve a purpose. 142a1fb - 🔥 Remove UnitsNet.Serialization.JsonNet.CompatibilityTests
@angularsen Hi!
I've lost track of this one a bit, but how shall we go on? It would be good if we could finally come to a conclusion (and get V5 out!)
There's also a build failure now that I think is unrelated to the PR.
Sorry for late reply, I've been missing out on some email notifications on this repo lately. I'll need to revisit this and refresh my brain on it, will get back to you.
@angularsen It would be good this could be taken on hand some time soon. I'm actually waiting for a v5.0 release due to some other internal changes that make stuff easier (and of course the included fixes).
Thank you for the reminder, I pushed some minor fixes and I think this is ready to merge now. Awesome job!
By the way, v5 is already out as a alpha prerelease nuget. It does lag behind master on new units though, because each merge to update that branch incur the mother of all merge conflicts. 🙈
I still have a few breaking changes I'd like to get around to before stabilizing the v5 nuget, but other than that it should be good to use already.
I merged v5 into master now by the way. Nuget on the way out.
I merged v5 into master now by the way. Nuget on the way out.
Great. I'll start testing it asap. We're preparing a major release in dotnet/iot that will bring a bunch of breaking changes, and therefore is a good time to also upgrade UnitsNet.