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.

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

Conversation

lipchev
Copy link
Collaborator

@lipchev lipchev commented Apr 12, 2025

As mentioned in #1509 , since all unit-related operations ultimately end up with a call to the ResourceManager expecting a CultureInfo, there is no point in using an IFormatProvider for the method parameter.

This does not affect the Quantity.Parse methods, which also depend on the NumberFormat (the IFormatProvider parameter is safe-cast when calling the UnitParser).

@lipchev
Copy link
Collaborator Author

lipchev commented Apr 12, 2025

@angularsen If you agree with the premise, this should be easy to review - I intentionally did not touch anything but the parameter types and the xml-docs.

This is certainly a breaking change, but my guess is that it won't be that bad- this should only affect users that are passing a parameter of type IFormatProvider. This should still work with a concrete culture (e.g. CultureInfo.InvariantCulture) or null.

If this is all acceptable, then I'll just do a quick-reformat, before adding the rest of the modifications to the UnitAbbreviationsCache and the UnitParser (fixing the TODOs) from my other branch (hopefully there wouldn't be anything preventing me from doing so).

Copy link
Owner

@angularsen angularsen left a comment

Choose a reason for hiding this comment

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

I'm having second thoughts, see comments

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

@angularsen angularsen closed this Apr 15, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants