-
Notifications
You must be signed in to change notification settings - Fork 393
Fix UnitParser.ParseUnit for multiple-worded units #254
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -68,6 +68,12 @@ public class SpeedTests : SpeedTestsBase | |
|
||
protected override double MillimetersPerHourInOneMeterPerSecond => 3600000; | ||
|
||
protected override double InchesPerSecondInOneMeterPerSecond => 39.3700787; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yes I messed up with the branches. I'm not familiar using Git yet. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Oh yeah, I might as well ask you how should I have done the two pieces of work? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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:
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. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. |
||
|
||
protected override double YardsPerMinuteInOneMeterPerSecond => 65.6167979; | ||
|
||
protected override double YardsPerSecondInOneMeterPerSecond => 1.09361330; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I double checked these values though, looks good! |
||
|
||
[Test] | ||
public void DurationSpeedTimesEqualsLength() | ||
{ | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -54,13 +54,14 @@ internal static TUnit ParseUnit<TUnit>([NotNull] string str, | |
|
||
const string exponentialRegex = @"(?:[eE][-+]?\d+)?)"; | ||
|
||
string regexString = string.Format(@"(?:\s*(?<value>[-+]?{0}{1}{2}{3})?{4}{5}", | ||
string regexString = string.Format(@"(?:\s*(?<value>[-+]?{0}{1}{2}{3})?", | ||
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 | ||
@"(and)?,?", // allow "and" & "," separators between quantities | ||
@"(?<invalid>[a-z]*)?"); // capture invalid input | ||
@"(?<unit>[^\d]+)"); // capture Unit (non-numeric) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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 Something like this should work:
The
And you'd need to refactor the
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Whenever I add There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. ok.. so I tried hacking into the UnitSystem class to get all the abbreviation given a UnitClasses type. I think having a single regex or parse algorithm for all the Unit types will have its limitations. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Ah, you raise some good points here.
You are right, I was probably too quick here, but
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 With the start/end boundaries, a
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 What do you think? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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). :) There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. No worries, thanks for posting an update! Did my proposal make sense to you? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Yes! especially that sentence. I'll try using your regex and add unit tests for it when I get the time. 👍 |
||
|
||
//remove separators | ||
str = str.Replace("and", ""); | ||
|
||
List<TUnit> quantities = ParseWithRegex(regexString, str, parseUnit, formatProvider); | ||
if (quantities.Count == 0) | ||
|
@@ -89,15 +90,7 @@ private static List<TUnit> ParseWithRegex<TUnit>(string regexString, string str, | |
|
||
string valueString = groups["value"].Value; | ||
string unitString = groups["unit"].Value; | ||
if (groups["invalid"].Value != "") | ||
{ | ||
var newEx = new UnitsNetException("Invalid string detected: " + groups["invalid"].Value); | ||
newEx.Data["input"] = str; | ||
newEx.Data["matched value"] = valueString; | ||
newEx.Data["matched unit"] = unitString; | ||
newEx.Data["formatprovider"] = formatProvider?.ToString(); | ||
throw newEx; | ||
} | ||
|
||
if ((valueString == "") && (unitString == "")) continue; | ||
|
||
try | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice!