Deprecate QuantityType to better support custom quantities #864

generateui · 2020-12-04T17:22:34Z

See #858. Turns out, QuantityInfo is already sufficient to represent a type of quantity. Hence, the solution is simple: remove QuantityType.

Tests pass, but I have not done a thorough check to actually merge the PR. Let's discuss first if this solution is desirable. To me, it solves the problem - QuantityType is gone, extensibility on QuantityType is now irrelevant.

Furthermore, users of all quantities can now use Quantity.ByName to enumerate all quantities.

Please let me know if this is the direction this library want to go into. If so, I'll publish follow up changes to ready up for a merge.

angularsen

I think you made an excellent observation that we can reuse QuantityInfo instead of QuantityType for enumerating quantities, looking up one statically via Length.Info or dynamically via Quantity.Infos.

As in my comments, I propose we revert to keep old functionality and keep it backwards compatible, but mark it obsolete. Unit tests should test both old and new, until removed.

Then update #180 with what to remove and cleanup when we bump the major version.
There is a bunch of other stuff waiting there already.

What I didn't immediately see, is how do we achieve more extensibility than before?

Wiki describes how to extend with custom units, for converting or parsing, but it doesn't describe how to add custom quantities. One specific thing I believe is missing is how to add HowMuch to Quantity.Infos, which is currently an array. I can't immediately think of other scenarios where extensibility is lacking, but I'm sure there is something.

I'm totally fine with attacking that point later, just wanted to bring it up to hear if you had any thoughts of it.

CodeGen/Generators/UnitsNetGen/QuantityGenerator.cs

UnitsNet.Tests/GeneratedCode/IQuantityTests.g.cs

UnitsNet/QuantityTypeConverter.cs

generateui · 2020-12-05T11:51:29Z

Right, I forgot UnitsNet supports graceful removal of features. Obsoleting first makes total sense. Agreed that only when actual removal is done, tests should be removed.

Would you please update #180, when the time is there?

What I didn't immediately see, is how do we achieve more extensibility than before?

We don't. We just remove a blocker on extensibility by changing the fact that the library relies on QuantityType. The PR effectively moves QuantityType to a role where it is not required to use any more, which is needed for extended quantities. Extended quantities can't provide a correct QuantityType value, and as such using api's where QuantityType is used will fail. That is what this PR fixes.

I have opened #865 to discuss extensibility features in a tracking issue.

I will update the PR to support obsoletion and initial addition of the QuantityInfo apis.

generateui · 2020-12-05T13:18:44Z

I will update the PR to support obsoletion and initial addition of the QuantityInfo apis.

Done. Could you re-review? Thanks!

angularsen

Good progress, some findings below.

UnitsNet.Tests/CustomQuantities/HowMuch.cs

UnitsNet.Tests/GeneratedCode/IQuantityTests.g.cs

UnitsNet.Tests/GeneratedCode/TestsBase/AccelerationTestsBase.g.cs

UnitsNet.Tests/QuantityInfoTest.cs

UnitsNet.Tests/QuantityTest.cs

UnitsNet.WindowsRuntimeComponent/GeneratedCode/Quantity.g.cs

UnitsNet/CustomCode/Quantity.cs

UnitsNet/IQuantity.cs

UnitsNet/QuantityTypeConverter.cs

angularsen · 2020-12-08T20:40:35Z

Seems like tests fail on NullReferenceException in the build:

https://ci.appveyor.com/project/angularsen/unitsnet/builds/36677657

Failed UnitsNet.Tests.CustomCode.ThermalConductivityTests.ToString_ReturnsValueAndUnitAbbreviationInCurrentCulture [1 ms]
  Error Message:
   System.TypeInitializationException : The type initializer for 'UnitsNet.ThermalConductivity' threw an exception.
---- System.TypeInitializationException : The type initializer for 'UnitsNet.Quantity' threw an exception.
-------- System.TypeInitializationException : The type initializer for 'UnitsNet.Acceleration' threw an exception.
------------ System.NullReferenceException : Object reference not set to an instance of an object.
  Stack Trace:
     at UnitsNet.ThermalConductivity..ctor(Double value, ThermalConductivityUnit unit) in C:\projects\unitsnet\UnitsNet\GeneratedCode\Quantities\ThermalConductivity.g.cs:line 71
   at UnitsNet.Tests.ThermalConductivityTestsBase.ToString_ReturnsValueAndUnitAbbreviationInCurrentCulture() in C:\projects\unitsnet\UnitsNet.Tests\GeneratedCode\TestsBase\ThermalConductivityTestsBase.g.cs:line 360
----- Inner Stack Trace -----
   at UnitsNet.QuantityInfo..ctor(String name, UnitInfo[] unitInfos, Enum baseUnit, IQuantity zero, BaseDimensions baseDimensions) in C:\projects\unitsnet\UnitsNet\QuantityInfo.cs:line 99
   at UnitsNet.QuantityInfo`1..ctor(String name, UnitInfo`1[] unitInfos, TUnit baseUnit, IQuantity`1 zero, BaseDimensions baseDimensions) in C:\projects\unitsnet\UnitsNet\QuantityInfo.cs:line 238
   at UnitsNet.ThermalConductivity..cctor() in C:\projects\unitsnet\UnitsNet\GeneratedCode\Quantities\ThermalConductivity.g.cs:line 56
----- Inner Stack Trace -----
   at UnitsNet.Acceleration.FromMetersPerSecondSquared(QuantityValue meterspersecondsquared) in C:\projects\unitsnet\UnitsNet\GeneratedCode\Quantities\Acceleration.g.cs:line 358
   at UnitsNet.Tests.DecimalOverloadTests.CreatingQuantityWithDoubleBackingFieldFromDecimalReturnsCorrectValue() in C:\projects\unitsnet\UnitsNet.Tests\DecimalOverloadTests.cs:line 13
----- Inner Stack Trace -----
   at UnitsNet.QuantityInfo..ctor(String name, UnitInfo[] unitInfos, Enum baseUnit, IQuantity zero, BaseDimensions baseDimensions) in C:\projects\unitsnet\UnitsNet\QuantityInfo.cs:line 99
   at UnitsNet.QuantityInfo`1..ctor(String name, UnitInfo`1[] unitInfos, TUnit baseUnit, IQuantity`1 zero, BaseDimensions baseDimensions) in C:\projects\unitsnet\UnitsNet\QuantityInfo.cs:line 238
   at UnitsNet.Acceleration..cctor() in C:\projects\unitsnet\UnitsNet\GeneratedCode\Quantities\Acceleration.g.cs:line 53
  Failed UnitsNet.Tests.CustomCode.ThermalConductivityTests.ConversionRoundTrip [1 ms]
  Error Message:

…work

generateui · 2020-12-12T14:57:13Z

I have fixed some warnings and fought a little fight with Visual Studio, resulting in #pragma littering. That seems to be fixed now (I still wonder why previous week the build succeeded and now a reboot and restart reports all these warnings as errors. The ways of VS move in mysterious ways).

I can't reproduce the error. Tracing it back points to

            if (Quantity.QuantityTypeByName.TryGetValue(name, out var quantityType))
                QuantityType = quantityType;
            else
                QuantityType = QuantityType.Undefined;

in the ctor of public QuantityInfo([NotNull] string name, [NotNull] UnitInfo[] unitInfos, [NotNull] Enum baseUnit, [NotNull] IQuantity zero, [NotNull] BaseDimensions baseDimensions). Yet, this dictionary is initialized and all tests pass locally. Can you take a look?

Pass QuantityType as argument with default value, instead of looking up in Quantity.

angularsen · 2020-12-12T21:49:26Z

I pushed some fixes on a cyclic static ctor call, and fixed some test cases.
It builds and all tests pass locally with build.bat now.

codecov · 2020-12-12T21:57:41Z

Codecov Report

Merging #864 (eb2f1e1) into master (4993a2a) will decrease coverage by 0.36%.
The diff coverage is 80.39%.

@@            Coverage Diff             @@
##           master     #864      +/-   ##
==========================================
- Coverage   83.09%   82.73%   -0.37%     
==========================================
  Files         287      287              
  Lines       42987    43308     +321     
==========================================
+ Hits        35720    35829     +109     
- Misses       7267     7479     +212

Impacted Files	Coverage Δ
UnitsNet/UnitConverter.cs	`82.35% <ø> (ø)`
UnitsNet/GeneratedCode/Quantity.g.cs	`52.00% <1.81%> (-29.41%)`	⬇️
UnitsNet/CustomCode/Quantity.cs	`81.08% <100.00%> (-3.13%)`	⬇️
...nitsNet/GeneratedCode/Quantities/Acceleration.g.cs	`82.56% <100.00%> (+0.09%)`	⬆️
...et/GeneratedCode/Quantities/AmountOfSubstance.g.cs	`82.88% <100.00%> (+0.09%)`	⬆️
...tsNet/GeneratedCode/Quantities/AmplitudeRatio.g.cs	`77.77% <100.00%> (+0.15%)`	⬆️
UnitsNet/GeneratedCode/Quantities/Angle.g.cs	`82.01% <100.00%> (+0.09%)`	⬆️
...tsNet/GeneratedCode/Quantities/ApparentEnergy.g.cs	`77.24% <100.00%> (+0.15%)`	⬆️
...itsNet/GeneratedCode/Quantities/ApparentPower.g.cs	`77.77% <100.00%> (+0.15%)`	⬆️
UnitsNet/GeneratedCode/Quantities/Area.g.cs	`84.46% <100.00%> (+0.08%)`	⬆️
... and 100 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 4993a2a...eb2f1e1. Read the comment docs.

angularsen · 2020-12-12T22:03:47Z

AppVeyor is happy, I'll try to re-review this soon.

generateui · 2020-12-13T14:56:37Z

How come the cyclic ctor call did not explode on my machine? I ran all tests.

angularsen · 2020-12-13T15:32:09Z

Try running build.bat. That excludes any quirks by Visual Studio and Rider, using dotnet CLI to run tests.

One thing is that the order of tests. If any test in the same app domain (test project) called Quantity type, then Quantity static ctor would successfully init.
If a test called Temperature.Info first, then it would cause problems.

My best guess is that you were "lucky" to not get that particular test execution order.

generateui · 2020-12-13T15:46:30Z

Gotcha, thanks. Is there anything you want me to do to move forward on this PR?

angularsen · 2020-12-13T20:22:16Z

No, I just need to read through the whole PR again. Maybe tomorrow.

Replace MSBuildTreatWarningsAsErrors with TreatWarningsAsErrors Demote nullability errors to warnings in CodeGen until fixed.

angularsen · 2020-12-16T23:55:42Z

@generateui I think this is ready to merge! Just waiting for AppVeyor to do its thing. It has been awfully slow lately.

angularsen · 2020-12-16T23:56:30Z

I was able to sort out the pragma stuff, see #872 .

angularsen · 2020-12-17T09:45:53Z

Quantity.g.cs had 30% lower test coverage, but we can sort that out later. I'd rather merge this beast now :D

If I read it right, the method is not covered by tests:
FromQuantityInfo

https://codecov.io/gh/angularsen/UnitsNet/pull/864/diff?src=pr&el=tree#diff-VW5pdHNOZXQvR2VuZXJhdGVkQ29kZS9RdWFudGl0eS5nLmNz

angularsen · 2020-12-17T22:34:06Z

Nuget is on the way out, thank you for the contribution!

Release UnitsNet/4.79.0 · angularsen/UnitsNet

generateui · 2020-12-19T20:07:54Z

This is awesome! Thanks for your close support.

remove QuantityType

2474ec1

angularsen requested changes Dec 4, 2020

View reviewed changes

generateui mentioned this pull request Dec 5, 2020

Support extensibility use cases #865

Closed

obsolete QuantityType instead of removing it straight away

cd40484

angularsen requested changes Dec 5, 2020

View reviewed changes

resolve code review feedback

7396796

Ruud Poutsma added 2 commits December 12, 2020 15:24

use #pragma to disable 612 & 618, since directory-wide setting don't …

64bc2a5

…work

I love how arbitrary the compiler reports warnings as errors

7d980c0

angularsen added 3 commits December 12, 2020 22:38

Fix cyclic static ctor call between QuantityInfo and Quantity

5f50e1f

Pass QuantityType as argument with default value, instead of looking up in Quantity.

Fix test cases

d100101

Fix Quantity.Names

1f3ae12

angularsen changed the title ~~remove QuantityType~~ Deprecate QuantityType to support custom quantities by name Dec 12, 2020

angularsen changed the title ~~Deprecate QuantityType to support custom quantities by name~~ Deprecate QuantityType to better support custom quantities Dec 12, 2020

angularsen mentioned this pull request Dec 16, 2020

Redesign UnitSystem to support non-SI systems and configurable default units #709

Closed

8 tasks

angularsen added 2 commits December 16, 2020 23:34

Remove pragma statements on obsolete warns

7d82bc9

Fix obsolete warnings with Directory.Build.prop

878d8a2

Replace MSBuildTreatWarningsAsErrors with TreatWarningsAsErrors Demote nullability errors to warnings in CodeGen until fixed.

angularsen mentioned this pull request Dec 16, 2020

Remove pragma statements on obsolete warns #872

Merged

angularsen added 2 commits December 17, 2020 00:22

Merge remote-tracking branch 'origin/master' into no-quantity-type

29002c5

Regen code

f6f7547

angularsen added 3 commits December 17, 2020 00:47

Minor cleanup

daa7528

Fix GetAttribute implementation

c4989f2

Typo

eb2f1e1

angularsen merged commit c46d064 into angularsen:master Dec 17, 2020

generateui deleted the no-quantity-type branch December 19, 2020 20:07

tmilnthorp mentioned this pull request Jan 26, 2022

Deprecate more usage of QuantityType #1019

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Deprecate QuantityType to better support custom quantities #864

Deprecate QuantityType to better support custom quantities #864

generateui commented Dec 4, 2020

angularsen left a comment

generateui commented Dec 5, 2020 •

edited

Loading

generateui commented Dec 5, 2020

angularsen left a comment

angularsen commented Dec 8, 2020

generateui commented Dec 12, 2020

angularsen commented Dec 12, 2020

codecov bot commented Dec 12, 2020 •

edited

Loading

angularsen commented Dec 12, 2020

generateui commented Dec 13, 2020

angularsen commented Dec 13, 2020 •

edited

Loading

generateui commented Dec 13, 2020

angularsen commented Dec 13, 2020

angularsen commented Dec 16, 2020

angularsen commented Dec 16, 2020

angularsen commented Dec 17, 2020

angularsen commented Dec 17, 2020

generateui commented Dec 19, 2020

Deprecate QuantityType to better support custom quantities #864

Deprecate QuantityType to better support custom quantities #864

Conversation

generateui commented Dec 4, 2020

angularsen left a comment

Choose a reason for hiding this comment

generateui commented Dec 5, 2020 • edited Loading

generateui commented Dec 5, 2020

angularsen left a comment

Choose a reason for hiding this comment

angularsen commented Dec 8, 2020

generateui commented Dec 12, 2020

angularsen commented Dec 12, 2020

codecov bot commented Dec 12, 2020 • edited Loading

Codecov Report

angularsen commented Dec 12, 2020

generateui commented Dec 13, 2020

angularsen commented Dec 13, 2020 • edited Loading

generateui commented Dec 13, 2020

angularsen commented Dec 13, 2020

angularsen commented Dec 16, 2020

angularsen commented Dec 16, 2020

angularsen commented Dec 17, 2020

angularsen commented Dec 17, 2020

generateui commented Dec 19, 2020

generateui commented Dec 5, 2020 •

edited

Loading

codecov bot commented Dec 12, 2020 •

edited

Loading

angularsen commented Dec 13, 2020 •

edited

Loading