Skip to content

Replaced the IFormatProvider parameter with CultureInfo for all unit-related operations #1542

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

Closed
wants to merge 1 commit into from
+1,435 −1,405
Closed

Replaced the IFormatProvider parameter with CultureInfo for all unit-related operations #1542

Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
20 changes: 10 additions & 10 deletions CodeGen/Generators/UnitsNetGen/QuantityGenerator.cs
View file
Original file line number Diff line number Diff line change
@@ -396,10 +396,10 @@ public static string GetAbbreviation({_unitEnumName} unit)
/// </summary>
/// <param name=""unit"">Unit to get abbreviation for.</param>
/// <returns>Unit abbreviation string.</returns>
/// <param name=""provider"">Format to use for localization. Defaults to <see cref=""CultureInfo.CurrentCulture"" /> if null.</param>
public static string GetAbbreviation({_unitEnumName} unit, IFormatProvider? provider)
/// <param name=""culture"">The localization culture. Defaults to <see cref=""CultureInfo.CurrentCulture"" /> if null.</param>
public static string GetAbbreviation({_unitEnumName} unit, CultureInfo? culture)
{{
return UnitsNetSetup.Default.UnitAbbreviations.GetDefaultAbbreviation(unit, provider);
return UnitsNetSetup.Default.UnitAbbreviations.GetDefaultAbbreviation(unit, culture);
}}
#endregion
@@ -557,18 +557,18 @@ public static bool TryParse([NotNullWhen(true)]string? str, IFormatProvider? pro
/// Parse a unit string.
/// </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.CurrentCulture"" /> if null.</param>
/// <param name=""culture"">The localization culture. Defaults to <see cref=""CultureInfo.CurrentCulture"" /> if null.</param>
/// <example>
/// Length.ParseUnit(""m"", CultureInfo.GetCultureInfo(""en-US""));
/// </example>
/// <exception cref=""ArgumentNullException"">The value of 'str' cannot be null. </exception>
/// <exception cref=""UnitsNetException"">Error parsing string.</exception>
public static {_unitEnumName} ParseUnit(string str, IFormatProvider? provider)
public static {_unitEnumName} ParseUnit(string str, CultureInfo? culture)
{{
return UnitsNetSetup.Default.UnitParser.Parse<{_unitEnumName}>(str, provider);
return UnitsNetSetup.Default.UnitParser.Parse<{_unitEnumName}>(str, culture);
}}
/// <inheritdoc cref=""TryParseUnit(string,IFormatProvider,out UnitsNet.Units.{_unitEnumName})""/>
/// <inheritdoc cref=""TryParseUnit(string,CultureInfo?,out UnitsNet.Units.{_unitEnumName})""/>
public static bool TryParseUnit([NotNullWhen(true)]string? str, out {_unitEnumName} unit)
{{
return TryParseUnit(str, null, out unit);
@@ -583,10 +583,10 @@ public static bool TryParseUnit([NotNullWhen(true)]string? str, out {_unitEnumNa
/// <example>
/// Length.TryParseUnit(""m"", CultureInfo.GetCultureInfo(""en-US""));
/// </example>
/// <param name=""provider"">Format to use when parsing number and unit. Defaults to <see cref=""CultureInfo.CurrentCulture"" /> if null.</param>
public static bool TryParseUnit([NotNullWhen(true)]string? str, IFormatProvider? provider, out {_unitEnumName} unit)
/// <param name=""culture"">The localization culture. Defaults to <see cref=""CultureInfo.CurrentCulture"" /> if null.</param>
public static bool TryParseUnit([NotNullWhen(true)]string? str, CultureInfo? culture, out {_unitEnumName} unit)
{{
return UnitsNetSetup.Default.UnitParser.TryParse<{_unitEnumName}>(str, provider, out unit);
return UnitsNetSetup.Default.UnitParser.TryParse<{_unitEnumName}>(str, culture, out unit);
}}
#endregion
14 changes: 9 additions & 5 deletions UnitsNet/CustomCode/Quantities/Length.extra.cs
View file
Original file line number Diff line number Diff line change
@@ -83,8 +83,9 @@ public static bool TryParseFeetInches(string? str, out Length result, IFormatPro
return true;

var quantityParser = UnitsNetSetup.Default.QuantityParser;
string footRegex = quantityParser.CreateRegexPatternForUnit(LengthUnit.Foot, formatProvider, matchEntireString: false);
string inchRegex = quantityParser.CreateRegexPatternForUnit(LengthUnit.Inch, formatProvider, matchEntireString: false);
var unitLocalizationCulture = formatProvider as CultureInfo;
string footRegex = quantityParser.CreateRegexPatternForUnit(LengthUnit.Foot, unitLocalizationCulture, matchEntireString: false);
string inchRegex = quantityParser.CreateRegexPatternForUnit(LengthUnit.Inch, unitLocalizationCulture, matchEntireString: false);

// Match entire string exactly
string pattern = $@"^(?<negativeSign>\-?)(?<feet>{footRegex})\s?(?<inches>{inchRegex})$";
@@ -154,10 +155,13 @@ public override string ToString()
/// </param>
public string ToString(IFormatProvider? cultureInfo)
{
cultureInfo = cultureInfo ?? CultureInfo.CurrentCulture;
if (cultureInfo is not CultureInfo unitLocalizationCulture)
{
cultureInfo = unitLocalizationCulture = CultureInfo.CurrentCulture;
}

var footUnit = Length.GetAbbreviation(LengthUnit.Foot, cultureInfo);
var inchUnit = Length.GetAbbreviation(LengthUnit.Inch, cultureInfo);
var footUnit = Length.GetAbbreviation(LengthUnit.Foot, unitLocalizationCulture);
var inchUnit = Length.GetAbbreviation(LengthUnit.Inch, unitLocalizationCulture);

// Note that it isn't customary to use fractions - one wouldn't say "I am 5 feet and 4.5 inches".
// So inches are rounded when converting from base units to feet/inches.
9 changes: 6 additions & 3 deletions UnitsNet/CustomCode/Quantities/Mass.extra.cs
View file
Original file line number Diff line number Diff line change
@@ -88,10 +88,13 @@ public override string ToString()
/// </param>
public string ToString(IFormatProvider? cultureInfo)
{
cultureInfo = cultureInfo ?? CultureInfo.CurrentCulture;
if (cultureInfo is not CultureInfo unitLocalizationCulture)
{
cultureInfo = unitLocalizationCulture = CultureInfo.CurrentCulture;
}

var stoneUnit = Mass.GetAbbreviation(MassUnit.Stone, cultureInfo);
var poundUnit = Mass.GetAbbreviation(MassUnit.Pound, cultureInfo);
var stoneUnit = Mass.GetAbbreviation(MassUnit.Stone, unitLocalizationCulture);
var poundUnit = Mass.GetAbbreviation(MassUnit.Pound, unitLocalizationCulture);

// Note that it isn't customary to use fractions - one wouldn't say "I am 11 stone and 4.5 pounds".
// So pounds are rounded here.
12 changes: 6 additions & 6 deletions UnitsNet/CustomCode/Quantity.cs
View file
Original file line number Diff line number Diff line change
@@ -98,15 +98,15 @@ public static IQuantity From(double value, string quantityName, string unitName)
/// This will fail if more than one unit across all quantities share the same unit abbreviation.<br/>
/// Prefer <see cref="From(double,System.Enum)"/> or <see cref="From(double,string,string)"/> instead.
/// </remarks>
/// <param name="formatProvider">The format provider to use for lookup. Defaults to <see cref="CultureInfo.CurrentCulture" /> if null.</param>
/// <param name="culture">The localization culture. Defaults to <see cref="CultureInfo.CurrentCulture" /> if null.</param>
/// <param name="value">Numeric value.</param>
/// <param name="unitAbbreviation">Unit abbreviation, such as "kg" for <see cref="MassUnit.Kilogram"/>.</param>
/// <returns>An <see cref="IQuantity"/> object.</returns>
/// <exception cref="UnitNotFoundException">Unit abbreviation is not known.</exception>
/// <exception cref="AmbiguousUnitParseException">Multiple units found matching the given unit abbreviation.</exception>
public static IQuantity FromUnitAbbreviation(IFormatProvider? formatProvider, double value, string unitAbbreviation)
public static IQuantity FromUnitAbbreviation(CultureInfo? culture, double value, string unitAbbreviation)
{
return From(value, UnitParser.GetUnitFromAbbreviation(unitAbbreviation, formatProvider).Value);
return From(value, UnitParser.GetUnitFromAbbreviation(unitAbbreviation, culture).Value);
}

/// <summary>
@@ -156,15 +156,15 @@ public static bool TryFromUnitAbbreviation(double value, string unitAbbreviation
/// This will fail if more than one unit across all quantities share the same unit abbreviation.<br/>
/// Prefer <see cref="From(double,System.Enum)"/> or <see cref="From(double,string,string)"/> instead.
/// </remarks>
/// <param name="formatProvider">The format provider to use for lookup. Defaults to <see cref="CultureInfo.CurrentCulture" /> if null.</param>
/// <param name="culture">The localization culture. Defaults to <see cref="CultureInfo.CurrentCulture" /> if null.</param>
/// <param name="value">Numeric value.</param>
/// <param name="unitAbbreviation">Unit abbreviation, such as "kg" for <see cref="MassUnit.Kilogram"/>.</param>
/// <param name="quantity">The quantity if successful, otherwise null.</param>
/// <returns>True if successful.</returns>
/// <exception cref="ArgumentException">Unit value is not a known unit enum type.</exception>
public static bool TryFromUnitAbbreviation(IFormatProvider? formatProvider, double value, string unitAbbreviation, [NotNullWhen(true)] out IQuantity? quantity)
public static bool TryFromUnitAbbreviation(CultureInfo? culture, double value, string unitAbbreviation, [NotNullWhen(true)] out IQuantity? quantity)
{
if (UnitParser.TryGetUnitFromAbbreviation(unitAbbreviation, formatProvider, out UnitInfo? unitInfo))
if (UnitParser.TryGetUnitFromAbbreviation(unitAbbreviation, culture, out UnitInfo? unitInfo))
{
return TryFrom(value, unitInfo.Value, out quantity);
}
8 changes: 4 additions & 4 deletions UnitsNet/CustomCode/QuantityParser.cs
View file
Original file line number Diff line number Diff line change
@@ -154,7 +154,7 @@ public bool TryParse<TQuantity, TUnitType>(string? str,

internal string CreateRegexPatternForUnit<TUnitType>(
TUnitType unit,
IFormatProvider? formatProvider,
CultureInfo? formatProvider,
bool matchEntireString = true)
where TUnitType : struct, Enum
{
@@ -186,7 +186,7 @@ private TQuantity ParseWithRegex<TQuantity, TUnitType>(string valueString,
where TUnitType : struct, Enum
Copy link
Owner

Choose a reason for hiding this comment

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

Why isn't this IFormatProvider also changed to CultureInfo?
double.Parse can take a CultureInfo, so why should we not just standardize on CultureInfo here and other places?

I tried reading up a bit on IFormatProvider, and its remarks section: https://learn.microsoft.com/en-us/dotnet/api/system.iformatprovider?view=net-9.0#remarks

The whole purpose of .NET standardizing on IFormatProvider, seems to me, is to support different custom formatters for string.Format(), ToString() etc.

There are 3 built in types: NumberFormatInfo, DateTimeFormatInfo and CultureInfo.

CultureInfo holds both number formatting and date formatting, which makes this whole thing a bit weird:

var culture = CultureInfo.GetCultureInfo("en-US");

// Equivalent, so double.Parse() can take both CultureInfo and NumberFormatInfo
double.Parse("5.5", culture);
double.Parse("5.5", culture.NumberFormat); 

// Only works if CurrentCulture is compatible, so it basically ignores incompatible `IFormatProvider` implementations
double.Parse("5.5", culture.DateTimeFormat);

This is because double.Parse() calls NumberFormatInfo.GetInstance(provider) to select NumberFormatInfo when given either CultureInfo or NumberFormatInfo, otherwise it falls back to CurrentCulture.

public static double Parse(String s, NumberStyles style, IFormatProvider provider) {
    NumberFormatInfo.ValidateParseStyleFloatingPoint(style);
    return Parse(s, style, NumberFormatInfo.GetInstance(provider));
}

https://github.com/microsoft/referencesource/blob/master/mscorlib/system/double.cs#L235-L251

https://github.com/microsoft/referencesource/blob/master/mscorlib/system/globalization/numberformatinfo.cs#L285-L310

Long story short, I have a feeling it's maybe not a good idea for us to ditch IFormatProvider in our public methods, even if we do require CultureInfo in the end. Similar to how .NET's own types standardize on IFormatProvider and has internal logic to obtain the compatible formatter, if any, or fallback to CurrentCulture.

To follow .NET conventions, I think we should just should do the same and cast internally, ignoring anything except CultureInfo for localization, and falling back to CurrentCulture.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

The reason why IFormatProvider is used in ToString and Parse is because they don't have any assumptions about whether the CultureInfo or NumberFormat (or the DateInfoFormat) is required by the target type.
On the other hand, CultureInfo is used for localizing strings (not sure if this is changing with the StringLocalizer) where only the culture name is the key.

In our case Mass.Zero.ToString should take an IFormatProvider, passing it to the Value (which uses the associated NumberFormat) and but type-casting it to a CultureInfo when accessing the ResourceDictionary.

Anyways, if you don't want to change it that's ok, I use nothing but InvariantCulture when dealing with the units.

Copy link
Owner

Choose a reason for hiding this comment

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

That's my point, double.Parse indeed has an assumption on the formatter it wants, but it still supports IFormatProvider in order to accept both NumberFormatInfo and CultureInfo. I guess several reasons, but consistency and also to make it easier for the developer to just pass whatever they have, instead of having to do myCulture.NumberFormat every time.

A similar point goes for us, since c# code typically passes IFormatProvider around, they'll have to start casting to CultureInfo just to satisfy our public signature. I don't see much benefit to require a concrete type, but I see how it can cause annoying friction.

Let's stay with IFormatProvider.

{
var value = double.Parse(valueString, ParseNumberStyles, formatProvider);
var parsedUnit = _unitParser.Parse<TUnitType>(unitString, formatProvider);
var parsedUnit = _unitParser.Parse<TUnitType>(unitString, formatProvider as CultureInfo);
return fromDelegate(value, parsedUnit);
}

@@ -207,7 +207,7 @@ private bool TryParseWithRegex<TQuantity, TUnitType>(string? valueString,
if (!double.TryParse(valueString, ParseNumberStyles, formatProvider, out var value))
return false;

if (!_unitParser.TryParse<TUnitType>(unitString, formatProvider, out var parsedUnit))
if (!_unitParser.TryParse<TUnitType>(unitString, formatProvider as CultureInfo, out TUnitType parsedUnit))
return false;

result = fromDelegate(value, parsedUnit);
@@ -246,7 +246,7 @@ private static bool TryExtractValueAndUnit(Regex regex, string str, [NotNullWhen

private string CreateRegexPatternForQuantity<TUnitType>(IFormatProvider? formatProvider) where TUnitType : struct, Enum
{
var unitAbbreviations = _unitAbbreviationsCache.GetAllUnitAbbreviationsForQuantity(typeof(TUnitType), formatProvider);
var unitAbbreviations = _unitAbbreviationsCache.GetAllUnitAbbreviationsForQuantity(typeof(TUnitType), formatProvider as CultureInfo);
var pattern = GetRegexPatternForUnitAbbreviations(unitAbbreviations);

// Match entire string exactly
Loading