Skip to content
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.

Sign up for GitHub

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

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

Closed
wants to merge 6 commits into from
Closed

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
b77e05b
Fix parsing feet/inches when thousands separator is '
pgrawehr May 22, 2020
c571add
Add extra locale test
pgrawehr May 26, 2020
4de3c57
Clarify and extend test purpose
pgrawehr May 28, 2020
54a8df1
Adjust to Nullable-Changes
pgrawehr May 28, 2020
0d24b46
Ensure this works regardless of whatever kind of apostrophe is used
pgrawehr May 28, 2020
310db24
Use predefined enum type for what we want
pgrawehr May 28, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
30 changes: 30 additions & 0 deletions UnitsNet.Tests/QuantityIFormattableTests.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -65,6 +65,36 @@ public void SFormatEqualsSignificantDigits(string sFormatString, string expected
Assert.Equal(expected, length.ToString(sFormatString, NumberFormatInfo.InvariantInfo));
}

/// <summary>
/// This verifies that the culture is correctly considered when formatting objects with an explicit culture.
/// </summary>
[Fact]
public void FormattingUsesSuppliedLocale()
{
Temperature t = Temperature.FromDegreesCelsius(2012.1234);
CultureInfo c = new CultureInfo("de-CH", false);
string formatted = string.Format(c, "{0:g}", t);
// Let's be very explicit here
Assert.Equal("2" + c.NumberFormat.NumberGroupSeparator + "012" + c.NumberFormat.NumberDecimalSeparator + "12 °C", formatted);
}

/// <summary>
/// This verifies that the culture is correctly considered when using <see cref="FormattableString.ToString(IFormatProvider)"/>
/// </summary>
[Fact]
public void FormatStringWorksWithSuppliedLocale()
{
Temperature t = Temperature.FromDegreesCelsius(2012.1234);
CultureInfo c = new CultureInfo("de-CH", false);

FormattableString f = $"{t:g}";
Assert.Equal("2" + c.NumberFormat.NumberGroupSeparator + "012" + c.NumberFormat.NumberDecimalSeparator + "12 °C", f.ToString(c));

// 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.
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Remove the commented code from this PR.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

}

[Fact]
public void UFormatEqualsUnitToString()
{
Expand Down
24 changes: 23 additions & 1 deletion UnitsNet/CustomCode/Quantities/Length.extra.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -59,6 +59,27 @@ public static Length ParseFeetInches([NotNull] string str, IFormatProvider? form
return result;
}

/// <summary>
/// Try to parse a string with one or two quantities of the format "&lt;quantity&gt; &lt;unit&gt;".
/// </summary>
/// <param name="str">String to parse. Typically in the form: {number} {unit}</param>
/// <param name="provider">Format to use when parsing number and unit. Defaults to <see cref="CultureInfo.CurrentUICulture" /> if null.</param>
/// <param name="allowedNumberStyles">Allowed number styles</param>
/// <param name="result">Resulting unit quantity if successful.</param>
/// <returns>True if successful, otherwise false.</returns>
/// <example>
/// Length.Parse("5.5 m", new CultureInfo("en-US"));
/// </example>
private static bool TryParse(string? str, IFormatProvider? provider, NumberStyles allowedNumberStyles, out Length result)
{
return QuantityParser.Default.TryParse<Length, LengthUnit>(
str,
provider,
From,
allowedNumberStyles,
out result);
}

/// <summary>
/// Special parsing of feet/inches strings, commonly used.
/// 2 feet 4 inches is sometimes denoted as 2′−4″, 2′ 4″, 2′4″, 2 ft 4 in.
Expand All @@ -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))
Copy link
Owner

@angularsen angularsen May 30, 2020 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Owner

@angularsen angularsen Jun 1, 2020 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

#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.

return true;

var quantityParser = QuantityParser.Default;
Expand Down
34 changes: 30 additions & 4 deletions UnitsNet/CustomCode/QuantityParser.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -69,10 +69,21 @@ internal TQuantity Parse<TQuantity, TUnitType>([NotNull] string str,
return ParseWithRegex(valueString!, unitString!, fromDelegate, formatProvider);
}

internal bool TryParse<TQuantity, TUnitType>(string? str,
IFormatProvider? formatProvider,
[NotNull] QuantityFromDelegate<TQuantity, TUnitType> fromDelegate,
out TQuantity result)
where TQuantity : struct, IQuantity
where TUnitType : struct, Enum
{
return TryParse(str, formatProvider, fromDelegate, ParseNumberStyles, out result);
}

[SuppressMessage("ReSharper", "UseStringInterpolation")]
internal bool TryParse<TQuantity, TUnitType>(string? str,
IFormatProvider? formatProvider,
[NotNull] QuantityFromDelegate<TQuantity, TUnitType> fromDelegate,
NumberStyles allowedNumberStyles,
out TQuantity result)
where TQuantity : struct, IQuantity
where TUnitType : struct, Enum
Expand All @@ -94,21 +105,22 @@ internal bool TryParse<TQuantity, TUnitType>(string? str,
if (!TryExtractValueAndUnit(regex, str, out var valueString, out var unitString))
return false;

return TryParseWithRegex(valueString, unitString, fromDelegate, formatProvider, out result);
return TryParseWithRegex(valueString, unitString, fromDelegate, formatProvider, allowedNumberStyles, out result);
}

/// <summary>
/// Workaround for C# not allowing to pass on 'out' param from type Length to IQuantity, even though the are compatible.
/// </summary>
[SuppressMessage("ReSharper", "UseStringInterpolation")]
internal bool TryParse<TQuantity, TUnitType>([NotNull] string str,
internal bool TryParse<TQuantity, TUnitType>(string? str,
IFormatProvider? formatProvider,
[NotNull] QuantityFromDelegate<TQuantity, TUnitType> fromDelegate,
NumberStyles allowedNumberStyles,
out IQuantity? result)
where TQuantity : struct, IQuantity
where TUnitType : struct, Enum
{
if (TryParse(str, formatProvider, fromDelegate, out TQuantity parsedQuantity))
if (TryParse(str, formatProvider, fromDelegate, allowedNumberStyles, out TQuantity parsedQuantity))
{
result = parsedQuantity;
return true;
Expand All @@ -118,6 +130,19 @@ internal bool TryParse<TQuantity, TUnitType>([NotNull] string str,
return false;
}

/// <summary>
/// Workaround for C# not allowing to pass on 'out' param from type Length to IQuantity, even though the are compatible.
/// </summary>
internal bool TryParse<TQuantity, TUnitType>(string? str,
IFormatProvider? formatProvider,
[NotNull] QuantityFromDelegate<TQuantity, TUnitType> fromDelegate,
out IQuantity? result)
where TQuantity : struct, IQuantity
where TUnitType : struct, Enum
{
return TryParse(str, formatProvider, fromDelegate, ParseNumberStyles, out result);
}

internal string CreateRegexPatternForUnit<TUnitType>(
TUnitType unit,
IFormatProvider? formatProvider,
Expand Down Expand Up @@ -164,13 +189,14 @@ private bool TryParseWithRegex<TQuantity, TUnitType>(string? valueString,
string? unitString,
QuantityFromDelegate<TQuantity, TUnitType> fromDelegate,
IFormatProvider? formatProvider,
NumberStyles allowedNumberStyles,
out TQuantity result)
where TQuantity : struct, IQuantity
where TUnitType : struct, Enum
{
result = default;

if (!double.TryParse(valueString, ParseNumberStyles, formatProvider, out var value))
if (!double.TryParse(valueString, allowedNumberStyles, formatProvider, out var value))
return false;

if (!_unitParser.TryParse<TUnitType>(unitString, formatProvider, out var parsedUnit))
Expand Down