Splitting UnitSystem class into more focused classes #511

tmilnthorp · 2018-10-11T15:18:55Z

No description provided.

tmilnthorp · 2018-10-11T15:26:30Z

Interesting those checks fail on my machine, but pass when run by themselves. Have you seen anything like this before? I can't find any errant setting of the DefaultCulture

tmilnthorp · 2018-10-11T15:40:24Z

Ah. Found it. Some tests switch the DefaultCulture to CultureInfo.InvariantCulture and then do a UnitAbbreviationsCache.MapUnitToAbbreviation( LengthUnit.Meter, "m_s" ) for example.

This makes "m_s" the only abbreviation for CultureInfo.InvariantCulture so ToString and Parse expect that when tests use the InvariantCulture. The UnitAbbreviationsCache is now globally persistent so it doesn't go away. I think that actually makes sense but what do you think?

angularsen · 2018-10-11T16:01:30Z

Yes, testing static types is always fun. I believe we can instruct xunit to run certain groups of tests in a separate appdomain than other tests - and if I recall correctly we already did such an effort back when either writing the tests or switching from nunit to xunit.

Custom Test Collections
https://xunit.github.io/docs/running-tests-in-parallel.html#parallelism-in-test-frameworks

But, why is this a problem now and not before? Wasn't UnitSystem using static cache per cultures already?

GetDefaultAbbreviation<TUnitType> should recursively call itself rather than the int version

Making parser and unit abbreviation caches non-static and making a static "Default" property for singleton access.

tmilnthorp · 2018-10-11T20:37:54Z

When in doubt, make them instance classes with a singleton pattern for maximum flexibility 😃

angularsen · 2018-10-11T22:26:22Z

:D It usually helps, but we still need a test that covers setting the static property and then verifying that ToString() on quantities are globally affected. Singleton or not, that access will be static.

Merge v4 into UnitSystemSplit

Matching behavior in tests to previous ones. Adding [Collection(nameof(UnitSystemFixture))] to prevent running in parallel as other tests were modifying the default abbreviation cache and causing wrong formats to be provided

tmilnthorp · 2018-10-12T15:11:29Z

Ok, I changed the tests back to match the previous behavior. I found the real issue -it was a multi-threading problem! Only saw it sometimes, especially in debug. I had to add [Collection(nameof(UnitSystemFixture))] to ParseTests, otherwise other tests were modifying GlobalConfiguration.DefaultCulutre at the same time the default parser was accessing it. Everything matches the old behavior and passes now 👍

Removing commented code

angularsen

Looks good to me, you can merge this now and split the test class later if you want.

angularsen · 2018-10-12T16:36:17Z

UnitsNet.Tests/UnitSystemTests.cs

@@ -72,12 +72,14 @@ private enum CustomUnit
            IEnumerable<TUnitType> unitValues)
            where TUnitType : Enum
        {
+            var culture = GetCulture(cultureName);


We should rename/split this test class too.

angularsen · 2018-10-12T16:38:26Z

UnitsNet/CustomCode/UnitValueAbbreviationLookup.cs

@@ -45,7 +45,7 @@ internal string[] GetAllAbbreviations()
            return unitToAbbreviationMap.Values.SelectMany((abbreviations) =>
            {
                return abbreviations;
-            } ).ToArray();
+            } ).Distinct().ToArray();


Ah nice, we do indeed have duplicates in certain quantities.

angularsen · 2018-10-12T16:41:22Z

[C:\projects\unitsnet\UnitsNet.WindowsRuntimeComponent\UnitsNet.WindowsRuntimeComponent.csproj]
GeneratedCode\Quantities\ElectricCharge.WindowsRuntimeComponent.g.cs(95,129): warning CS1574: XML comment has cref attribute 'UnitSystem' that could not be resolved

A lot of spam on this one, maybe fix that first?
https://ci.appveyor.com/project/angularsen/unitsnet/builds/19466282

Fixing XML documentation

Breaking apart/renaming test classes to match library

tmilnthorp · 2018-10-12T17:33:29Z

Ok. Done. I do have a quick question though: any reason why GetDefaultAbbreviation was chosen to return "(no abbreviation for ... when one doesn't exist?

I would expect an exception, or at least returning string.Empty to indicate failure. Otherwise you have to check if the string starts with "(no abbreviation for " for a failure.

Perhaps throw an exception and add a bool TryGetDefaultAbbreviation method? Or would you prefer returning an empty string?

Making internal method a bool Try for ease of use

angularsen · 2018-10-12T17:49:01Z

On the phone 📱. I don't recall, sounds reasonable to throw I think. It would only happen for json not defining abbreviations (do we have any like that?) I think. Even when adding units at runtime you do that by adding at least one abbrev.

Use exception type with Assert.Throws

tmilnthorp · 2018-10-12T17:55:39Z

I think it would indeed be an exceptional case. I will add that.

Throw NotImplementedException when no abbreviations are added for a unit. Adding a test for all units to make sure one is specified.

angularsen · 2018-10-12T18:24:30Z

The ones I can think of are those without any abbrevs, possibly empty string, for example decimal fraction

tmilnthorp · 2018-10-12T18:27:20Z

Ok. Did that. Let me know what you think before I merge it since it's a bigger change.

angularsen

Ready to go, just a couple of comments you can consider.

angularsen · 2018-10-13T09:58:59Z

UnitsNet.Tests/CustomCode/ParseTests.cs

@@ -93,16 +92,15 @@ public void ParseWithCultureUsingSpaceAsThousandSeparators(string s, double expe
        }

        [Theory]
-        [InlineData("500.005.050,001 m", "UnitsNet.UnitsNetException")]
+        [InlineData("500.005.050,001 m", typeof(UnitsNetException))]


Nice upgrade

angularsen · 2018-10-13T10:00:26Z

UnitsNet.Tests/CustomCode/ParseTests.cs

        {
            var numberFormat = (NumberFormatInfo) CultureInfo.InvariantCulture.NumberFormat.Clone();
            numberFormat.NumberGroupSeparator = " ";
            numberFormat.NumberDecimalSeparator = ".";

-            string actual = AssertExceptionAndGetFullTypeName(() => Length.Parse(s, numberFormat));
-            Assert.Equal(expected, actual);
+            Assert.Throws(expectedExceptionType, () => Length.Parse(s, numberFormat));


Much better!

angularsen · 2018-10-13T10:01:38Z

UnitsNet.Tests/GeneratedCode/AccelerationTestsBase.g.cs

@@ -327,5 +327,18 @@ public void UnitsDoesNotContainUndefined()
        {
            Assert.DoesNotContain(AccelerationUnit.Undefined, Acceleration.Units);
        }
+
+        [Fact]
+        public void AllUnitsHaveAtLeastOneAbbreviationSpecified()


angularsen · 2018-10-13T10:05:23Z

UnitsNet.Tests/GeneratedCode/AccelerationTestsBase.g.cs

+                if(unit == AccelerationUnit.Undefined)
+                    continue;
+
+                var defaultAbbreviation = UnitAbbreviationsCache.Default.GetDefaultAbbreviation(unit);


Just a thought, would Default be better named SystemDefault or CurrentCulture or something?
It's heavily based on CultureInfo.CurrentCulture and CultureInfo.CurrentUICulture behind the scenes.

angularsen · 2018-10-13T10:07:14Z

UnitsNet.Tests/GeneratedCode/AccelerationTestsBase.g.cs

+                if(unit == AccelerationUnit.Undefined)
+                    continue;
+
+                var defaultAbbreviation = UnitAbbreviationsCache.Default.GetDefaultAbbreviation(unit);


Also, it would be helpful to add a comment that we are testing that this does not throw exceptions. It might not be obvious to the reader. Removing the var defaultAbbreviation variable would also help highlight this.

angularsen

Merge when you want.

angularsen · 2018-10-13T12:38:14Z

I'm merging this now, so I can work on top of it in my strict parsing PR.
Feel free to address my comments in a separate PR.

# 4.0.0 Release This PR will serve as the list of items to complete and it will be updated to show the progress before finally merged into `master` when completed. We all have busy schedules, so **if you want to help move this work forward then that is much appreciated!** ## The main theme is to reduce binary size In two years it has grown from 280 kB to 1.4 MB and a lot of it is due to unnecessary syntactic sugar with many method overloads for various number types and nullable types - for every of our 800+ units! It simply adds up to a big total. These items are chosen from #180, trying to include as many of the low hanging fruits as possible, while still keeping the list short and realistic to complete in a reasonably short time - we are all busy in our daily lives. We can always have more major version bumps later than trying to perfect it all now. ## Feature complete before November, merged before mid-December I would like to aim for a "beta" pre-release nuget sometime in October with all the items completed, so we can have some time to test it before releasing the final, stable 4.0.0 version before Christmas holidays. We should have the work items list more or less final before Monday, October 8th. ## Changes #### Added - [x] UnitSystem with a different meaning, now defines the base units for a unit system (#524) #### Removed - [x] Replace netstandard1.0 target with netstandard2.0, to avoid the extra dependencies (#477) - [x] Remove code marked as `[Obsolete]`, such as `VolumeUnit.Teaspoon` (#490) - [x] Remove nullable `From` factory methods (#483) - [x] Remove extension methods on _nullable_ number types (#483) - [x] Remove all number extension methods (#497) - [x] Remove `Length2d`, replaced by `Area` (#501) - [x] Remove static methods on `UnitSystem`, should use `UnitSystem.Default` instead (#496) - [x] Remove unit parameter from `ToString()` methods (#546) #### Changed - [x] Throw exception on NaN values in static constructor methods, like `FromMeters()` (#502, see #176 (comment)) - [x] Throw if unit was not specified when constructing quantities, (#499 - #389 (comment)) - [x] Stricter parsing (#343 and #180 (comment)) - [x] Remove unit parameter from `ToString()` methods (#546) - [x] Change Temperature arithmetic back to use base unit Kelvin (#550, see #518) #### Renamed - [x] Correct SingularName for some Flow unit definitions (#494, see #360) - [x] Split/rename UnitSystem into UnitParser, GlobalConfiguration, UnitAbbreviationsCache (#511) #### Fixed - [x] Search for any TODO comments in the code to address, remove comment and either fix or create issue (5d24432) - [x] Update README with v4 changes #498 - [x] Do not try/catch in UnitConverter.Try-methods (#506) - [x] Do not try/catch in `Length.TryParse()` and for other quantities (#507) - [x] Add/update changelog in wiki for v4 with some summary copied from this issue, some v3 to v4 migration instructions and briefly explain why some stuff were removed ## Milestones - [x] Finalize work items list (Monday, October 8) - [x] Release 4.0.0-beta1, feature complete (Wednesday, October 31) - [ ] ~Show release notes and upgrade guide when upgrading to 4.x nuget, if possible~ - [x] Release 4.0.0 (Monday, December 17)

Splitting UnitSystem class into more focused classes

1bf14a8

tmilnthorp added 2 commits October 11, 2018 13:07

79ce236

GetDefaultAbbreviation<TUnitType> should recursively call itself rather than the int version

f9c6790

Making parser and unit abbreviation caches non-static and making a static "Default" property for singleton access.

tmilnthorp added 2 commits October 12, 2018 09:38

1ada74a

Merge v4 into UnitSystemSplit

4a67832

Matching behavior in tests to previous ones. Adding [Collection(nameof(UnitSystemFixture))] to prevent running in parallel as other tests were modifying the default abbreviation cache and causing wrong formats to be provided

6d86563

Removing commented code

angularsen reviewed Oct 12, 2018

View reviewed changes

tmilnthorp added 3 commits October 12, 2018 13:15

Merge branch 'v4' into UnitSystemSplit

6af3595

2d9e9c2

Fixing XML documentation

e429c29

Breaking apart/renaming test classes to match library

53ba68d

Making internal method a bool Try for ease of use

8dd5866

Use exception type with Assert.Throws

b7523d2

Throw NotImplementedException when no abbreviations are added for a unit. Adding a test for all units to make sure one is specified.

angularsen reviewed Oct 13, 2018

View reviewed changes

angularsen approved these changes Oct 13, 2018

View reviewed changes

angularsen merged commit c2881b4 into angularsen:v4 Oct 13, 2018

angularsen mentioned this pull request Nov 8, 2018

v4.0.0 #487

Merged

25 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Splitting UnitSystem class into more focused classes #511

Splitting UnitSystem class into more focused classes #511

tmilnthorp commented Oct 11, 2018

tmilnthorp commented Oct 11, 2018

tmilnthorp commented Oct 11, 2018

angularsen commented Oct 11, 2018

tmilnthorp commented Oct 11, 2018

angularsen commented Oct 11, 2018

tmilnthorp commented Oct 12, 2018

angularsen left a comment

angularsen Oct 12, 2018

tmilnthorp Oct 12, 2018

angularsen Oct 12, 2018

angularsen commented Oct 12, 2018

tmilnthorp commented Oct 12, 2018

angularsen commented Oct 12, 2018

tmilnthorp commented Oct 12, 2018

angularsen commented Oct 12, 2018

tmilnthorp commented Oct 12, 2018

angularsen left a comment

angularsen Oct 13, 2018

angularsen Oct 13, 2018

angularsen Oct 13, 2018

angularsen Oct 13, 2018

angularsen Oct 13, 2018

angularsen left a comment

angularsen commented Oct 13, 2018

Splitting UnitSystem class into more focused classes #511

Splitting UnitSystem class into more focused classes #511

Conversation

tmilnthorp commented Oct 11, 2018

tmilnthorp commented Oct 11, 2018

tmilnthorp commented Oct 11, 2018

angularsen commented Oct 11, 2018

tmilnthorp commented Oct 11, 2018

angularsen commented Oct 11, 2018

tmilnthorp commented Oct 12, 2018

angularsen left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

angularsen commented Oct 12, 2018

tmilnthorp commented Oct 12, 2018

angularsen commented Oct 12, 2018

tmilnthorp commented Oct 12, 2018

angularsen commented Oct 12, 2018

tmilnthorp commented Oct 12, 2018

angularsen left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

angularsen left a comment

Choose a reason for hiding this comment

angularsen commented Oct 13, 2018