Fix TryParseFeetInches when current locale uses ' as number separator #794

pgrawehr · 2020-05-26T05:32:17Z

My current locale (de-CH) uses ' (an apostrophe) as thousands separator. This broke the unit test for the TryParseFeetInches method, because the input string 1'1" is ambiguous. Therefore I now removed the separator as valid input in a feet-inches combination. It's pretty unlikely that someone will need to parse 1,345'21".

codecov-commenter · 2020-05-26T07:00:49Z

Codecov Report

Merging #794 into master will increase coverage by 1.90%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##           master     #794      +/-   ##
==========================================
+ Coverage   78.60%   80.51%   +1.90%     
==========================================
  Files         280      280              
  Lines       41834    41850      +16     
==========================================
+ Hits        32884    33695     +811     
+ Misses       8950     8155     -795

Impacted Files	Coverage Δ
UnitsNet/CustomCode/Quantities/Length.extra.cs	`86.95% <100.00%> (+0.43%)`	⬆️
UnitsNet/CustomCode/QuantityParser.cs	`97.05% <100.00%> (+0.18%)`	⬆️
UnitsNet/QuantityFormatter.cs	`94.44% <0.00%> (-2.11%)`	⬇️
...tensions/GeneratedCode/NumberToAreaExtensions.g.cs	`100.00% <0.00%> (ø)`
...tensions/GeneratedCode/NumberToMassExtensions.g.cs	`100.00% <0.00%> (ø)`
...ensions/GeneratedCode/NumberToAngleExtensions.g.cs	`100.00% <0.00%> (ø)`
...ensions/GeneratedCode/NumberToForceExtensions.g.cs	`100.00% <0.00%> (ø)`
...ensions/GeneratedCode/NumberToLevelExtensions.g.cs	`100.00% <0.00%> (ø)`
...ensions/GeneratedCode/NumberToPowerExtensions.g.cs	`100.00% <0.00%> (ø)`
...ensions/GeneratedCode/NumberToRatioExtensions.g.cs	`100.00% <0.00%> (ø)`
... and 195 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 3b36fd9...310db24. Read the comment docs.

angularsen

Very nice, looks good to me. Just a couple minor comments.

UnitsNet/CustomCode/Quantities/Length.extra.cs

UnitsNet.Tests/QuantityIFormattableTests.cs

tmilnthorp · 2020-05-28T01:24:14Z

I'm not so certain that just ignoring it is a good idea. Looks like we're missing a couple tests though - for your case as well as a number separated by a '

When the thousands separator is ' (i.e. de-CH language settings) parsing 1'1" feet failed.

pgrawehr · 2020-05-28T17:00:41Z

I'm not so certain that just ignoring it is a good idea. Looks like we're missing a couple tests though - for your case as well as a number separated by a '

What test cases are you thinking of? If the number is i.e. embedded in quotes, then I think it's ok to have parsing fail, as quotes are not part of a number (except in the case where the quote is one of the valid separators).

tmilnthorp · 2020-05-28T17:54:20Z

I added an example here: #799. I'm confused now though because it passes :)

pgrawehr · 2020-05-28T18:25:31Z

@tmilnthorp : And I'm confused now because my updated test fails :-/

Output:

Error Message:
   Assert.Equal() Failure
           � (pos 1)
Expected: 2'012.12 øC
Actual:   2'012.12 øC
           � (pos 1)
  Stack Trace:
     at UnitsNet.Tests.QuantityIFormattableTests.FormatStringWorksWithSuppliedLocale() in C:\projects\unitsnet\UnitsNet.Tests\QuantityIFormattableTests.cs:line 90

The strings are exactly the same to me...

I'll check what's going on with your PR.

angularsen · 2020-05-28T18:31:24Z

Looks like an encoding issue. There are different apostrophe characters in different code sets.

tmilnthorp · 2020-05-28T18:48:50Z

Oh man. There's the right single quotation mark U+2019 and an apostrophe U+0027.

Sometimes, I feel like I know nothing 😆

angularsen · 2020-05-28T18:56:54Z

To be fair, xUnit COULD have been nice enough to help identify this by providing the character values when there is a single character off.

Different unicode characters may look the same on many fonts.

UnitsNet.Tests/QuantityIFormattableTests.cs

pgrawehr · 2020-05-29T04:46:21Z

$"{temperature}".ToString(myCulture)

The reason is: It's defined that way: https://docs.microsoft.com/en-us/dotnet/csharp/language-reference/language-specification/expressions#interpolated-strings

The type of $"..." is string, unless you immediatelly assign it to FormattableString. So the type of the expression is derived from the left-hand-side of the assigment.

angularsen · 2020-05-30T20:43:50Z

UnitsNet.Tests/QuantityIFormattableTests.cs

+
+            // This does not work. Looks like a compiler bug to me.
+            // string f2 = $"{t:g}".ToString(c);
+            // Assert.Equal("2'012.12 °C", f2.ToString(c)); // Actual value is formatted according to CurrentUiCulture.


The commented code does not work, because these two statements are equivalent:

string f2 = $"{t:g}".ToString(c); string f2 = t.ToString("g").ToString(c);

LINQPad sample

when CurrentUICulture is Norwegian and thousand separator is comma ,.

Temperature t = Temperature.FromDegreesCelsius(2012.1234); CultureInfo c = new CultureInfo("de-CH", false); FormattableString f = $"{t:g}"; string f2 = $"{t:g}".ToString(c); string f3 = t.ToString("g").ToString(c); var expected = "2" + c.NumberFormat.NumberGroupSeparator + "012" + c.NumberFormat.NumberDecimalSeparator + "12 °C"; (expected == f.ToString(c).Dump()).Dump(); (expected == f2.ToString(c).Dump()).Dump(); // Actual value is formatted according to CurrentUiCulture. (expected == f3.ToString(c).Dump()).Dump(); // Actual value is formatted according to CurrentUiCulture.

Output:

2’012.12 °C True 2,012.12 °C False 2,012.12 °C False

Remove the commented code from this PR.

As you've seen

FormattableString f = $"{t:g}"; string result = f.ToString(c);

is not equivalent to

string result = $"{t:g}".ToString(c);

which is what confused me (as this would be the case for every other expression).

I'll remove it, since the problem has nothing to do with this lib, but is a (weird but documented) feature of the CLR.

angularsen · 2020-05-30T20:52:57Z

UnitsNet/CustomCode/Quantities/Length.extra.cs

@@ -80,7 +101,8 @@ public static bool TryParseFeetInches(string? str, out Length result, IFormatPro
            str = str.Trim();

            // This succeeds if only feet or inches are given, not both
-            if (TryParse(str, formatProvider, out result))
+            // Do not allow thousands separator here, since it may be equal to the unit abbreviation (').
+            if (TryParse(str, formatProvider, NumberStyles.Float, out result))


As @tmilnthorp pointed out earlier, we should have unit tests that covers the new behavior for feet-inches. It should test what happens if you try to parse strings like these, with a culture that has apostrophe character as thousand separator. We could also test cultures with other thousand separators, like nb-NO Norwegian that has . as separator (instead of , in US English).

1'500' 4" 1'5002′−4″ 1'500 ft 4 in 1'5002′ 4″

My intuition tells me we should get an exception when parsing these, because the thousand separator is no longer allowed and in the first case the feet symbol is even listed twice.

I'll do that, but I feel we should somehow merge @tmilnthorp s PR with this one, otherwise it gets confusing.

I guess we can wait for #799 to merge first to reuse the combinatorial parameter refactoring that is about to be added there.

#799 was abandoned, please see #803 for supporting thousand separators in regex instead of not allowing them. I think that may be a better solution to this problem.

stale · 2020-08-01T00:36:24Z

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

pgrawehr force-pushed the ParserFix branch from e2b4f67 to e464f56 Compare May 26, 2020 06:51

angularsen requested changes May 27, 2020

View reviewed changes

UnitsNet/CustomCode/Quantities/Length.extra.cs Outdated Show resolved Hide resolved

UnitsNet.Tests/QuantityIFormattableTests.cs Show resolved Hide resolved

pgrawehr added 3 commits May 28, 2020 18:56

Fix parsing feet/inches when thousands separator is '

b77e05b

When the thousands separator is ' (i.e. de-CH language settings) parsing 1'1" feet failed.

Add extra locale test

c571add

Clarify and extend test purpose

4de3c57

pgrawehr force-pushed the ParserFix branch from fb69ec9 to 4de3c57 Compare May 28, 2020 16:57

Adjust to Nullable-Changes

54a8df1

pgrawehr added 2 commits May 28, 2020 21:46

Ensure this works regardless of whatever kind of apostrophe is used

0d24b46

Different unicode characters may look the same on many fonts.

Use predefined enum type for what we want

310db24

angularsen requested changes May 28, 2020

View reviewed changes

UnitsNet.Tests/QuantityIFormattableTests.cs Show resolved Hide resolved

UnitsNet.Tests/QuantityIFormattableTests.cs Show resolved Hide resolved

angularsen reviewed May 30, 2020

View reviewed changes

angularsen requested changes May 30, 2020

View reviewed changes

tmilnthorp mentioned this pull request Jun 1, 2020

Fix feet inches #803

Closed

pgrawehr mentioned this pull request Jul 26, 2020

Culture-Dependent parsing of feet/inches #817

Open

stale bot added the wontfix label Aug 1, 2020

stale bot closed this Aug 8, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix TryParseFeetInches when current locale uses ' as number separator #794

Fix TryParseFeetInches when current locale uses ' as number separator #794

pgrawehr commented May 26, 2020

codecov-commenter commented May 26, 2020 •

edited

Loading

angularsen left a comment

tmilnthorp commented May 28, 2020

pgrawehr commented May 28, 2020

tmilnthorp commented May 28, 2020

pgrawehr commented May 28, 2020

angularsen commented May 28, 2020 via email

tmilnthorp commented May 28, 2020

angularsen commented May 28, 2020 •

edited

Loading

pgrawehr commented May 29, 2020

angularsen May 30, 2020

angularsen May 30, 2020

pgrawehr May 31, 2020

angularsen May 30, 2020 •

edited

Loading

pgrawehr May 31, 2020

angularsen May 31, 2020

angularsen Jun 1, 2020 •

edited

Loading

stale bot commented Aug 1, 2020

Fix TryParseFeetInches when current locale uses ' as number separator #794

Fix TryParseFeetInches when current locale uses ' as number separator #794

Conversation

pgrawehr commented May 26, 2020

codecov-commenter commented May 26, 2020 • edited Loading

Codecov Report

angularsen left a comment

Choose a reason for hiding this comment

tmilnthorp commented May 28, 2020

pgrawehr commented May 28, 2020

tmilnthorp commented May 28, 2020

pgrawehr commented May 28, 2020

angularsen commented May 28, 2020 via email

tmilnthorp commented May 28, 2020

angularsen commented May 28, 2020 • edited Loading

pgrawehr commented May 29, 2020

angularsen May 30, 2020

Choose a reason for hiding this comment

LINQPad sample

angularsen May 30, 2020

Choose a reason for hiding this comment

pgrawehr May 31, 2020

Choose a reason for hiding this comment

angularsen May 30, 2020 • edited Loading

Choose a reason for hiding this comment

pgrawehr May 31, 2020

Choose a reason for hiding this comment

angularsen May 31, 2020

Choose a reason for hiding this comment

angularsen Jun 1, 2020 • edited Loading

Choose a reason for hiding this comment

stale bot commented Aug 1, 2020

codecov-commenter commented May 26, 2020 •

edited

Loading

angularsen commented May 28, 2020 •

edited

Loading

angularsen May 30, 2020 •

edited

Loading

angularsen Jun 1, 2020 •

edited

Loading