Fix UnitParser.ParseUnit for multiple-worded units #254

gojanpaolo · 2017-05-12T20:44:32Z

Changed regex to accept multiple worded non-numeric units
Used string.Replace to remove all known separators. This way, we
don't need to include it in our regex. Also the separator "," is already
defined in NumberFormatInfo numFormat
Removed code block for group since it will not go into this
anymore.
Added Unit test for Parsing Mass "Short Tons" and "Long Tons"

#253 1. Changed regex to accept multiple worded non-numeric units 2. Used string.Replace to remove all known separators. This way, we don't need to include it in our regex. Also the separator "," is already defined in NumberFormatInfo numFormat 3. Removed code block for <invalid> group since it will not go into this anymore. 4. Added Unit test for Parsing Mass "Short Tons" and "Long Tons"

Added: in/s yd/s yd/min

Added Speed Units

angularsen

The new units look good and could be merged, but since you have mixed that with the unit parsing change, you will first have to address the unit parsing comments or create a separate PR for the new units.

angularsen · 2017-05-12T22:18:51Z

UnitsNet/CustomCode/UnitParser.cs

-                @"(?<unit>[^\s\d,]+)", // capture Unit (non-whitespace) input
-                @"(and)?,?", // allow "and" & "," separators between quantities
-                @"(?<invalid>[a-z]*)?"); // capture invalid input
+                @"(?<unit>[^\d]+)"); // capture Unit (non-numeric)


This works with our current cases, but we might stumble onto problems here later if we get abbreviations with numbers in them. As an example, we currently only list m² in Area.Squaremeter, but for convenience sake we might want to also add m^2 so that they can parse both variants. The latter would not work with this implementation.
I'm fine with postponing that until we cross that bridge, but one option is to define the regex as a number immediately followed by any of the known unit abbreviations for the specified culture (IFormatProvider).

Something like this should work:

string valueRegex = $"{numRegex}{exponentialRegex}"; string unitRegex = "(m²|m\^2|cm²|cm\^2)"; string valueUnitRegex = $"(?<value>{valueRegex})\s?(?<unit>{unitRegex})"`;

The unitRegex string values can be enumerated by something like this:

var unitSystem = UnitSystem.GetCached(formatProvider); string[] abbrevsForAllUnits = Enum.GetValues(typeof(TUnit)) .Cast<TUnit>() .SelectMany(unitSystem.GetAllAbbreviations) .Select(Regex.Escape) .ToArray();

And you'd need to refactor the UnitParser.ParseUnit() method a bit:

Add where TUnit : struct, IComparable, IFormattable (should have been added to begin with)

Whenever I add where TUnit : struct, IComparable, IFormattable, I get a bunch of errors.
I think you're mistaking TUnit in UnitParser.ParseUnit() as the Unit Enums (i.e. LengthUnit, MassUnit, etc) but TUnit is actually the UnitClasses (Length, Mass).

ok.. so I tried hacking into the UnitSystem class to get all the abbreviation given a UnitClasses type.
The problem is, using the format of regex you suggested. It will fail on test "1ft 1invalid" since it recognizes the "INvalid" as inches. It also fails the test "1ft monkey 1in" since it recognizes "FT monkey".
I thought about restricting it to match only exact abbreviation but how will it handle something like "m" and "mm"?

I think having a single regex or parse algorithm for all the Unit types will have its limitations.
If we want the parsing to handle special cases then maybe it will be easier to implement one parse method for each Unit type.

Ah, you raise some good points here.

I think you're mistaking TUnit in UnitParser.ParseUnit() as the Unit Enums

You are right, I was probably too quick here, but UnitSystem should be able to provide this information as you found out.

It will fail on test "1ft 1invalid" since it recognizes the "INvalid" as inches. It also fails the test "1ft monkey 1in" since it recognizes "FT monkey". It also fails the test "1ft monkey 1in" since it recognizes "FT monkey".
I thought about restricting it to match only exact abbreviation but how will it handle something like "m" and "mm"?

I think your suggestion should work, we just need ensure we don't match on text that is not part of a valid pair of {value}{unit} or the accepted delimiters between pairs or between value and unit. I tried a few things and wound up with this: ^((?<value>\d+)\s?(?<unit>m²|m\^2|cm²|cm\^2|ft|in|m|mm)\s?)+$ and I added unit tests for the cases you listed above that you can view here: https://regex101.com/r/EMLIwt/1

With the start/end boundaries, a + repeater to match on at least one pair and an optional whitespace \s? between each pair, the regex now seems to properly match on full abbreviations, and it should not incorrectly match m on 1mm.

I think having a single regex or parse algorithm for all the Unit types will have its limitations.
If we want the parsing to handle special cases then maybe it will be easier to implement one parse method for each Unit type.

I hear you and I agree that might be something we eventually have to reach for, but I also do think my updated regex might just work. The upside of the complexity is that it allows for parsing any number of valid pairs of value+unit and that the implementation is uniform across units, so it is consistent and easier to reason about. The downside is the complexity of the regex itself, performance and the fact that it might not make sense or at least not be commonly used to parse all kinds of pairs of quantities from a string, like we are used to from 1ft 2in and 1' 2''.

What do you think?

Hi, I have been working on another project last week thus the late response. I'll get back to this once I finished the said project (maybe a couple weeks more). :)

No worries, thanks for posting an update! Did my proposal make sense to you?

it allows for parsing any number of valid pairs of value+unit and that the implementation is uniform across units, so it is consistent and easier to reason about.

Yes! especially that sentence. I'll try using your regex and add unit tests for it when I get the time. 👍

angularsen · 2017-05-12T22:21:10Z

UnitsNet.Tests/CustomCode/ParseTests.cs

+        [TestCase("333 long tn", Result = 333)]
+        public double ParseMassLongTon(string s)
+        {
+            return Mass.Parse(s).LongTons;


angularsen · 2017-05-12T22:23:07Z

UnitsNet.Tests/CustomCode/SpeedTests.cs

@@ -68,6 +68,12 @@ public class SpeedTests : SpeedTestsBase

        protected override double MillimetersPerHourInOneMeterPerSecond => 3600000;

+        protected override double InchesPerSecondInOneMeterPerSecond => 39.3700787;


I think you've mixed two pieces of work here. I saw you were working on the master branch, but it would probably be better for you to work on a feature branch to separate the two bulks of changes and create separate pull requests.
We can proceed like this if you are not familiar with how to do it, no biggie, but just a heads up for next time.

Yes I messed up with the branches. I'm not familiar using Git yet.

Oh yeah, I might as well ask you how should I have done the two pieces of work?
Should I have created two forks? one for the bugfix and one for the addfeature?

I would recommend googling for some guides on git, github, forks and pull requests. I haven't read this one, but at first glance it seems to cover the basics: https://gist.github.com/Chaser324/ce0505fbed06b947d962

The basic gist of it is:

Add official repo as remote so you can sync in changes, I prefer to call this origin and if I have a fork I call that my, but that is just my preference: git remote set-url origin https://github.com/anjdreas/unitsnet and git remote add my https://github.com/gojanpaolo/unitsnet

Create a local branch (based on origin/master) for any new work you want to create a pull request for, fix-unit-parsing or add-pascal-unit: git checkout -b fix-unit-parsing origin/master

Do your work as one or more commits

When you are ready for review, rebase your branch on top of any new commits on the official repo, then push that branch up to your github fork: git rebase origin/master && git push -u my

Visit github.com to create a pull request from that branch

It's a bit boilerplate the first time, especially setting up the remotes and stuff, but it gets easier with practice. You might also want to use some GUI clients to make things easier, but I personally prefer the command line.

And, for the second pull request, repeat all but the first step, this way both branches are based on origin/master and don't have any commits from the other branch.

angularsen · 2017-05-12T22:24:27Z

UnitsNet.Tests/CustomCode/SpeedTests.cs

+
+        protected override double YardsPerMinuteInOneMeterPerSecond => 65.6167979;
+
+        protected override double YardsPerSecondInOneMeterPerSecond => 1.09361330;


I double checked these values though, looks good!

angularsen · 2017-06-17T22:34:09Z

Still waiting for you to address my feedback. Also, it needs to be rebased on top of latest master as there is a merge conflict right now.

gojanpaolo · 2017-06-17T22:47:36Z

I think I will still be busy on projects at work for the next couple of months. :/

angularsen · 2017-08-04T06:17:57Z

Fixed by #265.

gojanpaolo added 3 commits May 12, 2017 15:44

Added Speed Units

937550a

Added: in/s yd/s yd/min

Merge pull request #1 from gojanpaolo/feature/add-SpeedUnit

6ae222d

Added Speed Units

angularsen requested changes May 12, 2017

View reviewed changes

bplubell mentioned this pull request Jul 10, 2017

Bugfix/parse mapped abbrev #265

Merged

angularsen closed this Aug 4, 2017

angularsen mentioned this pull request Jan 10, 2018

"Length.TryParse" parses invalid values #343

Closed

bplubell mentioned this pull request Jan 11, 2018

v5: Wishlist for breaking changes #180

Closed

13 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix UnitParser.ParseUnit for multiple-worded units #254

Fix UnitParser.ParseUnit for multiple-worded units #254

gojanpaolo commented May 12, 2017

angularsen left a comment

angularsen May 12, 2017

gojanpaolo May 13, 2017

gojanpaolo May 13, 2017

angularsen May 13, 2017

gojanpaolo May 21, 2017

angularsen May 21, 2017

gojanpaolo May 21, 2017 •

edited

Loading

angularsen May 12, 2017

angularsen May 12, 2017

gojanpaolo May 13, 2017

gojanpaolo May 13, 2017

angularsen May 13, 2017 •

edited

Loading

angularsen May 13, 2017 •

edited

Loading

angularsen May 12, 2017

angularsen commented Jun 17, 2017

gojanpaolo commented Jun 17, 2017

angularsen commented Aug 4, 2017

		@@ -68,6 +68,12 @@ public class SpeedTests : SpeedTestsBase

		protected override double MillimetersPerHourInOneMeterPerSecond => 3600000;

		protected override double InchesPerSecondInOneMeterPerSecond => 39.3700787;


		protected override double YardsPerMinuteInOneMeterPerSecond => 65.6167979;

		protected override double YardsPerSecondInOneMeterPerSecond => 1.09361330;

Fix UnitParser.ParseUnit for multiple-worded units #254

Fix UnitParser.ParseUnit for multiple-worded units #254

Conversation

gojanpaolo commented May 12, 2017

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

Choose a reason for hiding this comment

gojanpaolo May 21, 2017 • edited Loading

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 May 13, 2017 • edited Loading

Choose a reason for hiding this comment

angularsen May 13, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

angularsen commented Jun 17, 2017

gojanpaolo commented Jun 17, 2017

angularsen commented Aug 4, 2017

gojanpaolo May 21, 2017 •

edited

Loading

angularsen May 13, 2017 •

edited

Loading

angularsen May 13, 2017 •

edited

Loading