Skip to content

Return decimal for decimal-based quantities - #1074

Merged
angularsen merged 27 commits into
angularsen:release/v5from
pgrawehr:decimalConversion2
Nov 29, 2022
Merged

Return decimal for decimal-based quantities#1074
angularsen merged 27 commits into
angularsen:release/v5from
pgrawehr:decimalConversion2

Conversation

@pgrawehr

@pgrawehrpgrawehr commented Apr 17, 2022•
edited by angularsen
Loading

Copy link
Copy Markdown
Contributor

Fixes#1058

Return the correct value type for quantities that use decimal internally; Power, Information, BitRate.

  • Quantity properties return decimal or double based on internal value type.
  • IQuantity.Value returns QuantityValue, which supports both double and decimal.
  • QuantityValue: Implement IEquality, IComparable, IComparable.

@pgrawehrpgrawehr changed the title Test by hand (need to do all these changes in generator)Correct support for quantities with decimal base typeApr 17, 2022
@angularsen

angularsen commented Apr 21, 2022

Copy link
Copy Markdown
Owner

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

@angularsenangularsen mentioned this pull request Apr 21, 2022
Closed
@pgrawehr

pgrawehr commented Apr 22, 2022

Copy link
Copy Markdown
ContributorAuthor

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

angularsen
angularsen requested changes May 4, 2022
View reviewed changes

@angularsenangularsen left a comment

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wow, this is a ton of great work 👏
Some suggestions below, but it's close to ready!

Comment threadUnitsNet.Serialization.JsonNet.Tests/UnitsNetBaseJsonConverterTest.cs Outdated
Comment threadCodeGen/Generators/UnitsNetGen/QuantityGenerator.cs Outdated
Comment threadCodeGen/Generators/UnitsNetGen/QuantityGenerator.cs Outdated
Comment threadUnitsNet/GeneratedCode/Quantities/Acceleration.g.cs
Comment threadUnitsNet/GeneratedCode/Quantities/Acceleration.g.cs Outdated
Comment threadUnitsNet/QuantityValue.cs
Comment threadUnitsNet/QuantityValue.cs Outdated
Comment threadUnitsNet/QuantityValue.cs Outdated
Comment threadUnitsNet/QuantityValue.cs Outdated
Comment threadUnitsNet/QuantityValue.cs Outdated
@angularsen

angularsen commented May 22, 2022

Copy link
Copy Markdown
Owner

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

@pgrawehr
pgrawehr marked this pull request as ready for review May 29, 2022 12:54
@pgrawehr

pgrawehr commented May 29, 2022

Copy link
Copy Markdown
ContributorAuthor

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

@angularsen

angularsen commented May 29, 2022 via email

Copy link
Copy Markdown
Owner

@pgrawehr

pgrawehr commented May 30, 2022

Copy link
Copy Markdown
ContributorAuthor

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

@angularsen

angularsen commented May 30, 2022•
edited
Loading

Copy link
Copy Markdown
Owner

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.

@stalestaleBot added the wontfix label Aug 13, 2022
@lipchev

lipchev commented Aug 13, 2022

Copy link
Copy Markdown
Collaborator

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" />

@stalestaleBot removed the wontfix label Aug 13, 2022
@pgrawehr

pgrawehr commented Aug 13, 2022

Copy link
Copy Markdown
ContributorAuthor

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

@lipchev

lipchev commented Aug 13, 2022

Copy link
Copy Markdown
Collaborator

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

@pgrawehr

pgrawehr commented Aug 13, 2022•
edited
Loading

Copy link
Copy Markdown
ContributorAuthor

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

@lipchev

lipchev commented Aug 13, 2022

Copy link
Copy Markdown
Collaborator

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

@angularsenangularsen mentioned this pull request Aug 14, 2022
Closed
@pgrawehrpgrawehr mentioned this pull request Aug 23, 2022
Closed
@angularsen

angularsen commented Sep 2, 2022•
edited
Loading

Copy link
Copy Markdown
Owner

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

@pgrawehr
Remove implicit multiplication of QuantityValue
686d187
Even though it seems nice, it could probably cause quite
a bit of confusion.
@pgrawehr

pgrawehr commented Oct 3, 2022

Copy link
Copy Markdown
ContributorAuthor

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

@angularsen

angularsen commented Oct 31, 2022 via email

Copy link
Copy Markdown
Owner

@pgrawehr

pgrawehr commented Nov 29, 2022

Copy link
Copy Markdown
ContributorAuthor

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

@angularsen

angularsen commented Nov 29, 2022

Copy link
Copy Markdown
Owner

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.

@angularsenangularsen changed the title Correct support for quantities with decimal base typeReturn decimal for decimal-based quantitiesNov 29, 2022
@angularsen
angularsen merged commit 9bafef0 into angularsen:release/v5Nov 29, 2022
@angularsenangularsen mentioned this pull request Nov 29, 2022
Merged
angularsen added a commit that referenced this pull request Nov 29, 2022
@angularsen
v5 Release (#982)
9574d47
Fixes#180 Merging the v5 release branch.
It is still in alpha, but it is functional, nugets are published and there are not many planned breaking changes left.
By merging, all efforts moving forward are targeting v5 and this reduces friction:
- No more merge conflicts trying to forward port all changes to v5, instead cherry pick new units and fixes to v4 until v5 is fully stable.
- Contributors are having trouble building v4 locally due to `net40`, `net47` and Windows Runtime Component targets.
## 💥 Breaking changes
Default number format should be CultureInfo.CurrentCulture, not CurrentUICulture (#795)
Use CurrentCulture rather than CurrentUICulture (#986)
Return `QuantityValue` in `IQuantity` properties instead of `double` (#1074)
Return `decimal` in properties of `Power`, `BitRate` and `Information` quantities (#1074)
Fix singular name VolumeFlow.MillionUsGallonsPerDay
## 🔥 Removed Remove targets: net40, net47, Windows Runtime Component.
Remove `Undefined` enum value for all unit enum types
Remove QuantityType enum
Remove IQuantity.Units and .UnitNames
Remove IQuantity.ToString() overloads
Remove IEquatable<T> and equality operators/methods Remove GlobalConfiguration
Remove obsolete and deprecated code.
Remove Molarity ctor and operator overloads
Remove MinValue, MaxValue per quantity due to ambiguity
Remove string format specifiers: "v", "s"
json: Remove UnitsNetJsonConverter
## ✨ New
QuantityValue: Implement IEquality, IComparable, IFormattable
QuantityValue: 16 bytes instead of 40 bytes (#1084)
Add `[DataContract]` annotations (#972)
## ♻️ Improvements
Upgrade CodeGen, tests and sample apps to net6.0.
## 📝 JSON unit definition schema changes
Rename `BaseType` to `ValueType`, for values "double" and "decimal".
Rename `XmlDoc` to `XmlDocSummary`.
## TODO
Add back `IEquatable<T>`, but implement as strict equality with tuple of quantity name + unit + value.
#1017 (comment)
## Postponed for later
#1067
@angularsen

angularsen commented Nov 29, 2022

Copy link
Copy Markdown
Owner

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

Release UnitsNet/5.0.0-rc001 · angularsen/UnitsNet

@pgrawehr

pgrawehr commented Nov 30, 2022

Copy link
Copy Markdown
ContributorAuthor

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.

@angularsenangularsen added this to the 5.0 milestone Nov 30, 2022
@angularsenangularsen mentioned this pull request Feb 17, 2023
Closed
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@angularsenangularsenangularsen requested changes
@lipchevlipchevlipchev left review comments

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

5.0

Development

Successfully merging this pull request may close these issues.

3 participants

@pgrawehr@angularsen@lipchev