UnitsNet icon indicating copy to clipboard operation
UnitsNet copied to clipboard

Correct support for quantities with decimal base type

Open pgrawehr opened this issue 3 years ago • 14 comments

Fixes #1058

Proposal for fixing #1058. As per discussion:

  • IQuantity.Value returns QuantityValue. This implies the addition of some explicit interface implementations to the quantities, so that the default Value property 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.

pgrawehr avatar Apr 17 '22 08:04 pgrawehr

Will get to this, just a bit short on time these days.

angularsen avatar Apr 21 '22 21:04 angularsen

@angularsen No worries, take your time. Good coders are always busy ;-)

pgrawehr avatar Apr 22 '22 12:04 pgrawehr

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 avatar May 22 '22 13:05 angularsen

@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?

pgrawehr avatar May 29 '22 17:05 pgrawehr

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 avatar May 29 '22 17:05 angularsen

@angularsen The entire PR is intended to go to v5 only.

pgrawehr avatar May 30 '22 05:05 pgrawehr

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.

angularsen avatar May 30 '22 19:05 angularsen

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.

stale[bot] avatar Aug 13 '22 05:08 stale[bot]

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 avatar Aug 13 '22 07:08 lipchev

@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.

pgrawehr avatar Aug 13 '22 07:08 pgrawehr

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)..

lipchev avatar Aug 13 '22 07:08 lipchev

@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.

pgrawehr avatar Aug 13 '22 07:08 pgrawehr

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. :)

lipchev avatar Aug 13 '22 07:08 lipchev

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 avatar Sep 02 '22 21:09 angularsen

@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.

pgrawehr avatar Oct 03 '22 06:10 pgrawehr

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 avatar Oct 31 '22 14:10 angularsen

@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).

pgrawehr avatar Nov 29 '22 08:11 pgrawehr

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.

angularsen avatar Nov 29 '22 20:11 angularsen

I merged v5 into master now by the way. Nuget on the way out.

Release UnitsNet/5.0.0-rc001 · angularsen/UnitsNet

angularsen avatar Nov 29 '22 23:11 angularsen

I merged v5 into master now by the way. Nuget on the way out.

Release UnitsNet/5.0.0-rc001 · angularsen/UnitsNet

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.

pgrawehr avatar Nov 30 '22 07:11 pgrawehr