Bugfix/parse mapped abbrev #265

bplubell · 2017-07-09T19:33:30Z

So the direction I went was to change the regex to in the UnitParser.ParseUnit to match one of any of the abbreviations for the unit type. This seems more robust as it uses UnitSystem abbreviations and allows for more flexible abbreviations (either mapped at run time or generated).

Related issue: #264

Match exactly one of the set of allowable units

bplubell · 2017-07-10T16:22:58Z

I want to add a few more tests, so this should not be merged quite yet.
I did not realize that the solution to this was basically the same as the solution to #254 and #253 -- I am going to add tests to verify those issues are resolved as well.

angularsen · 2017-07-10T21:16:43Z

Been a bit busy, will get to this soon.

angularsen

Looking good, I like the approach. A couple of comments.

angularsen · 2017-07-11T17:19:51Z

UnitsNet.Tests/CustomCode/ParseTests.cs

+        public void ParseLengthUnitAbbreviationWithNumbers(string s)
+        {
+            UnitSystem.ClearCache();
+            UnitSystem unitSystem = UnitSystem.GetCached("en-US");


I just realized a better approach than using UnitSystem.GetCached() is to add a new ctor overload that allows you to disable default unit abbreviations from being loaded. I just added this in bf11a78. GetCached() is not ideal, because it is

Statically cached (problem if running tests in parallel) and it

Automatically loads all the units defined in .json

If Length.json already contained these units you are defining, you don't know for sure if the test passes due to calling MapUnitToAbbreviation or not.

So I would change this snippet to:

var unitSystem = new UnitSystem("en-US", loadDefaultAbbreviations: false); unitSystem.MapUnitToAbbreviation(UnitsNet.Units.LengthUnit.Meter, "m2");

angularsen · 2017-07-11T17:22:52Z

UnitsNet.Tests/CustomCode/ParseTests.cs

+        [InlineData("_")]
+        [InlineData("?")]
+        [InlineData("123")]
+        [InlineData("od to")]


I found this test case confusing. Does od to have a particular meaning or are you simply testing whitespace in the abbreviation?
Also, the method name implies testing with numbers but the test cases seem to test that various special characters in the abbreviation work.

Ah, indeed you are right. od to was simply to test a space and the name was confusing. I will change to "SpecialCharacters".

angularsen · 2017-07-11T22:43:52Z

UnitsNet/CustomCode/UnitParser.cs

+                .OrderByDescending(s => s.Length) // Important to order by length -- if "m" is before "mm" and the input is "mm", it will match just "m" and throw invalid string error
+                .ToArray();
+            // Escape special regex characters
+            for (int i = 0; i < unitAbbreviations.Length; i++)


How about using Regex.Escape() instead:
https://msdn.microsoft.com/en-us/library/system.text.regularexpressions.regex.escape.aspx

That looks like a much better approach. By the looks of it, I will still need a for loop though? If the project is ever updated to .NET Standard 2.0, the for loop can be replaced by:
string unitsRegex = $"({String.Join("|", unitAbbreviations.ForEach(s => Regex.Replace(s)))})";

angularsen · 2017-07-11T22:45:22Z

UnitsNet/CustomCode/UnitParser.cs

            string regexString = string.Format(@"(?:\s*(?<value>[-+]?{0}{1}{2}{3})?{4}{5}",
                numRegex, // capture base (integral) Quantity value
                exponentialRegex, // capture exponential (if any), end of Quantity capturing
                @"\s?", // ignore whitespace (allows both "1kg", "1 kg")
-                @"(?<unit>[^\s\d,]+)", // capture Unit (non-whitespace) input
+                $@"(?<unit>{unitsRegex})", // capture Unit (non-whitespace) input


The @ is no longer needed. Update comment to something like // capture Unit by a list of abbreviations

This foreach loop goes away with Regex.Escape. Will also update comment.

angularsen · 2017-07-11T22:47:41Z

UnitsNet/CustomCode/UnitSystem.cs

+
+            if (_unitTypeToUnitValueToAbbrevs.TryGetValue(unitType, out unitValueToAbbrevs))
+            {
+                foreach (var item in unitValueToAbbrevs)


Can simplify this to return unitValueToAbbrevs.Values.ToArray();

It is giving me a compile time error trying to do this. I think it is because unitValueToAbbrevs.Values is a collection of lists. Is there another way with LINQ that I have not found?

Oh, right. Try unitValueToAbbrevs.Values.SelectMany(x => x).ToArray()

Nice, I like it. A good snippet to remember.

angularsen · 2017-07-11T22:47:51Z

UnitsNet/CustomCode/UnitSystem.cs

+        /// <param name="unitValue">Enum value for unit.</param>
+        /// <returns>Unit abbreviations associated with unit.</returns>
+        [PublicAPI]
+        public string[] GetAllAbbreviations(Type unitType)


Neat, this method has been requested before.

angularsen · 2017-07-11T22:59:13Z

UnitsNet/CustomCode/UnitParser.cs

@@ -37,7 +37,8 @@ internal static class UnitParser
        internal static TUnit ParseUnit<TUnit>([NotNull] string str,
            [CanBeNull] IFormatProvider formatProvider,
            [NotNull] ParseUnit<TUnit> parseUnit,
-            [NotNull] Func<TUnit, TUnit, TUnit> add)
+            [NotNull] Func<TUnit, TUnit, TUnit> add,
+            Type unitType)


How about adding another generic parameter instead? And renaming TUnit to TQuantity, as per recent discussion on naming conventions in #260.
Suggestion: ParseUnit<TQuantity, TUnit>()

I think I follow... see if the next commits are what you are expecting. Another idea is to use an interface for quantity, then the TUnit argument can be used to determine the type with no need for a redundant parameter.

…ed-abbrev

angularsen · 2017-07-12T20:18:27Z

Let me know when it's ready for another review

bplubell · 2017-07-12T21:53:19Z

It should be ready to review again.

bplubell · 2017-07-19T13:57:06Z

@anjdreas This is ready for review.

angularsen · 2017-07-21T16:31:13Z

Hi, I'm on vacation trip until July 30. Will review when I get back.

angularsen · 2017-07-21T16:31:39Z

I meant July 25

bplubell · 2017-07-21T17:03:04Z

No worries; thanks for the update. Have a great trip!

TQuantity refers to the unit struct type.

Following naming convention #260

angularsen

This looks good to me! I did some fixes by committing directly onto your PR.

angularsen · 2017-07-30T10:58:02Z

UnitsNet/CustomCode/UnitParser.cs

+            // Escape special regex characters
+            for (int i = 0; i < unitAbbreviations.Length; i++)
+            {
+                unitAbbreviations[i] = Regex.Escape(unitAbbreviations[i]);


This for-loop can be removed in favor of

.... .Select(Regex.Escape) .ToArray();

above.

angularsen · 2017-07-30T11:16:40Z

UnitsNet/CustomCode/UnitParser.cs


    internal static class UnitParser
    {
        [SuppressMessage("ReSharper", "UseStringInterpolation")]
-        internal static TUnit ParseUnit<TUnit>([NotNull] string str,
+        internal static TQuantity ParseUnit<TQuantityEnum, TQuantity>([NotNull] string str,


I renamed the type parameters to match a naming convention.
Instead of TQuantity, TUnit it is now TQuantityEnum, TQuantity.

bplubell added 5 commits July 7, 2017 22:15

Add sample test for special characters and number

2e78d20

Custom/script code for resolving numbers in parsed units

fdd4335

Use fallback culture abbreviations if none exist for current culter

bead97e

Make unitsRegex more robust

13c9931

Match exactly one of the set of allowable units

Update generated code

d6b1ac4

Add parse test for unit with space in abbreviation

96dede1

angularsen requested changes Jul 11, 2017

View reviewed changes

bplubell added 6 commits July 11, 2017 19:35

Update ParseTest names to be more accurate

3a65835

Use .NET methods for escaping Regex metacharacters

c109cc9

Move argument for enum type to a generic parameter

2b8fdf9

Merge remote-tracking branch 'anjdreas/master' into bugfix/parse-mapp…

1744ce1

…ed-abbrev

Fix naming issue between UnitSystem ctor for LoadDefaultAbbreviations

6fa99ed

Simplify GetAllAbbreviations using awesome LINQ extensions

e6117cd

angularsen added 4 commits July 30, 2017 13:00

UnitParser: Replace for-loop with .Select()

922e71a

UnitParser: Rename to TQuantityEnum

ee3e16e

TQuantity refers to the unit struct type.

UnitParser: Rename TUnit to TQuantity

c1318b5

Following naming convention #260

UnitSystem: Remove unused code

3f434da

angularsen approved these changes Jul 30, 2017

View reviewed changes

angularsen merged commit 97c3f16 into angularsen:master Jul 30, 2017

bplubell mentioned this pull request Aug 3, 2017

Mass.Parse Error on ToString(MassUnit.ShortTon) and ToString(MassUnit.LongTon) #253

Closed

angularsen mentioned this pull request Aug 4, 2017

Fix UnitParser.ParseUnit for multiple-worded units #254

Closed

Bugfix/parse mapped abbrev #265

Bugfix/parse mapped abbrev #265

Uh oh!

Conversation

bplubell commented Jul 9, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

bplubell commented Jul 10, 2017

Uh oh!

angularsen commented Jul 10, 2017

Uh oh!

angularsen left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

angularsen commented Jul 12, 2017

Uh oh!

bplubell commented Jul 12, 2017

Uh oh!

bplubell commented Jul 19, 2017

Uh oh!

angularsen commented Jul 21, 2017

Uh oh!

angularsen commented Jul 21, 2017

Uh oh!

bplubell commented Jul 21, 2017

Uh oh!

angularsen left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

bplubell commented Jul 9, 2017 •

edited

Loading