Use CurrentCulture rather than CurrentUICulture #795

tmilnthorp · 2020-05-26T14:11:52Z

Fixes #787

codecov-commenter · 2020-05-26T14:21:48Z

Codecov Report

Merging #795 into master will not change coverage.
The diff coverage is 98.33%.

@@           Coverage Diff           @@
##           master     #795   +/-   ##
=======================================
  Coverage   80.50%   80.50%           
=======================================
  Files         280      280           
  Lines       41841    41841           
=======================================
  Hits        33686    33686           
  Misses       8155     8155

Impacted Files	Coverage Δ
UnitsNet/CustomCode/GlobalConfiguration.cs	`0.00% <0.00%> (ø)`
UnitsNet/CustomCode/Quantity.cs	`84.21% <ø> (ø)`
UnitsNet/CustomCode/UnitParser.cs	`86.51% <ø> (ø)`
...sNet/GeneratedCode/Quantities/SpecificEntropy.g.cs	`76.96% <ø> (ø)`
...tsNet/GeneratedCode/Quantities/SpecificVolume.g.cs	`73.61% <ø> (ø)`
...tsNet/GeneratedCode/Quantities/SpecificWeight.g.cs	`80.31% <ø> (ø)`
UnitsNet/GeneratedCode/Quantities/Speed.g.cs	`84.72% <ø> (ø)`
UnitsNet/GeneratedCode/Quantities/Temperature.g.cs	`76.26% <ø> (ø)`
...eneratedCode/Quantities/TemperatureChangeRate.g.cs	`77.44% <ø> (ø)`
...Net/GeneratedCode/Quantities/TemperatureDelta.g.cs	`76.96% <ø> (ø)`
... and 100 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 c2ebeb9...b3a784d. Read the comment docs.

angularsen · 2020-05-27T21:24:22Z

Looks good to me, but as you said. I too think this should be treated as a breaking change. It's quite likely that this will break something for someone.

I have no problems with bumping to major version, other than that there are other things to include when we first start doing a bump.

#563

angularsen · 2020-05-27T21:40:14Z

CodeGen/Generators/UnitsNetGen/StaticQuantityGenerator.cs

@@ -87,7 +87,7 @@ public static bool TryFrom(QuantityValue value, Enum unit, out IQuantity quantit
        /// <summary>
        ///     Try to dynamically parse a quantity string representation.
        /// </summary>
-        /// <param name=""formatProvider"">The format provider to use for lookup. Defaults to <see cref=""CultureInfo.CurrentUICulture"" /> if null.</param>
+        /// <param name=""formatProvider"">The format provider to use for lookup. Defaults to <see cref=""CultureInfo.CurrentCulture"" /> if null.</param>


Hm. What is correct here, if we can only specify a single culture?

My understanding:
CurrentUICulture is used for display language in the UI, which affects our abbreviation parsing.
CurrentCulture is used for date/number formatting, which affects our number parsing.

From my understanding, CurrentUICulture is for use with resource manager only. It's an odd one, as CurrentUICulture would be en-US even for someone in Britain (where I'd expect en-GB). Important if we ever want to distinguish meter from metre for example?

Well. Thought experiment:

I am Norwegian.

I configure my Windows region to Norway, so that Windows Store shows me content available in my region. I believe this is not exposed in .NET.

I configure my Windows regional format to English (Sweden) to get ISO dates instead of Norwegian dates, because I'm weird that way. I believe this configures CurrentCulture to en-SE.

I configure Windows display language to English (United States), because I prefer reading English. I believe this configures CurrentUICulture to en-US.

This is my actual current setup:

Interestingly, my country/region Norway is not visible in RegionInfo. In fact, it seems this choice is not available in .NET framework, only via WinSDK or P/Invoke.

So. In my particular Windows configuration. I want to read English, but I want date/numbers formatted in ISO (Sweden).

How should UnitsNet format the number and what language should it choose abbreviations from? I would prefer Swedish numbers and English abbreviations, but it seems I may be stuck with all Swedish in this PR proposal, right?

Interesting. They had to make it difficult didn't they 😂

What does DateTime.Now.ToString("F") give you?

From MSDN:

The ToString(String) method returns the string representation of a date and time value in a specific format that uses the formatting conventions of the current culture; for more information, see CultureInfo.CurrentCulture.

Result:

Thursday, May 28, 2020 9:30:26 PM Thursday, 28 May 2020 21:30:26 den 28 maj 2020 21:30:26

That looks like what I'd expect. So everything is formatted according to the given culture. This suggests to always use CurrentCulture, and I think we can agree that this is what should be used for formatting the number part. For the units, it's a bit more complicated and the docs are a bit less clear.

Now technically, the unit names are resource-lookups and should use CurrentUiCulture, but apparently, its not used that way (otherwise, the day and month names in the above example would have been english, regardless of the explicitly provided culture, because @tmilnthorp uses en-US as CurrentUiCulture [I assume]). It's not possible to provide two cultures to a ToString call. And it would be confusing to use one language's string formatting with another languages unit names.

IMHO, the correct behavior is to always use the provided culture for the whole formatting operation (number and unit name) and use CurrentCulture as the default.

I think you are right. We can only pass a single culture, this is the norm in .NET.

Seeing that .NET DateTime also uses the passed in culture, instead of trying to use CurrentUICulture for the text parts, we should probably do the same.
In a way, it makes sense that whatever culture you pass in, is used consistently for all the parts of the string formatting. It is easier to reason about, even if it leaves my Windows configuration edge case hanging in the cold. I'm fine with that.

Not sure if this has any relevance- but for my localized WPF applications - I tend to setup 4 things on login (culture is part of the user profile):

CultureInfo.DefaultThreadCurrentCulture = cultureInfo; CultureInfo.DefaultThreadCurrentUICulture = cultureInfo; Thread.CurrentThread.CurrentCulture = cultureInfo; Thread.CurrentThread.CurrentUICulture = cultureInfo;

..thinking that the UI sets the default culture for the UI threads- with the other one concerning the culture of any background threads. And from what I understand- you're saying that actually one is for the language-localization and the other for the formatting?
PS I'm in the exact same situation as @angularsen in terms of the regional settings (only the input is different)

@lipchev : You would normally do that, if your application supports localization. If it does not, you could still use localized number formatting. The CurrentUiCulture will have a fallback (usually english) if it doesn't exist, while the CurrentCulture is always completely defined. Using applications (Windows itself, but also Office or Visual Studio) in english while still using the locale's preferred number/date formatting is a very common scenario.

tmilnthorp · 2020-05-29T12:12:11Z

Perhaps we should move the abbreviations into a resource file.

stale · 2020-07-28T18:10:09Z

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.

angularsen · 2021-11-14T00:02:09Z

Replaced by #795, merged in v5.

Fixes #180 Merging the v5 release branch. It is still in alpha, but it is functional, nugets are published and there are not many planned breaking changes left. By merging, all efforts moving forward are targeting v5 and this reduces friction: - No more merge conflicts trying to forward port all changes to v5, instead cherry pick new units and fixes to v4 until v5 is fully stable. - Contributors are having trouble building v4 locally due to `net40`, `net47` and Windows Runtime Component targets. ## 💥 Breaking changes Default number format should be CultureInfo.CurrentCulture, not CurrentUICulture (#795) Use CurrentCulture rather than CurrentUICulture (#986) Return `QuantityValue` in `IQuantity` properties instead of `double` (#1074) Return `decimal` in properties of `Power`, `BitRate` and `Information` quantities (#1074) Fix singular name VolumeFlow.MillionUsGallonsPerDay ## 🔥 Removed Remove targets: net40, net47, Windows Runtime Component. Remove `Undefined` enum value for all unit enum types Remove QuantityType enum Remove IQuantity.Units and .UnitNames Remove IQuantity.ToString() overloads Remove IEquatable<T> and equality operators/methods Remove GlobalConfiguration Remove obsolete and deprecated code. Remove Molarity ctor and operator overloads Remove MinValue, MaxValue per quantity due to ambiguity Remove string format specifiers: "v", "s" json: Remove UnitsNetJsonConverter ## ✨ New QuantityValue: Implement IEquality, IComparable, IFormattable QuantityValue: 16 bytes instead of 40 bytes (#1084) Add `[DataContract]` annotations (#972) ## ♻️ Improvements Upgrade CodeGen, tests and sample apps to net6.0. ## 📝 JSON unit definition schema changes Rename `BaseType` to `ValueType`, for values "double" and "decimal". Rename `XmlDoc` to `XmlDocSummary`. ## TODO Add back `IEquatable<T>`, but implement as strict equality with tuple of quantity name + unit + value. #1017 (comment) ## Postponed for later #1067

Use CurrentCulture rather than CurrentUICulture

Unverified

This commit is not signed, but one or more authors requires that any commit attributed to them is signed.

Learn about vigilant mode

3a55d4b

angularsen added this to the 5.0 milestone May 27, 2020

angularsen added the breaking-change label May 27, 2020

angularsen reviewed May 27, 2020

View reviewed changes

angularsen mentioned this pull request May 27, 2020

Add support for standard numeric format strings #788

Merged

tmilnthorp added 2 commits May 29, 2020 10:40

Merge branch 'master' into Culture

Unverified

This commit is not signed, but one or more authors requires that any commit attributed to them is signed.

Learn about vigilant mode

ef68ef0

Merge fix

Unverified

This commit is not signed, but one or more authors requires that any commit attributed to them is signed.

Learn about vigilant mode

b3a784d

stale bot added the wontfix label Jul 28, 2020

tmilnthorp added pinned and removed wontfix labels Jul 28, 2020

lipchev mentioned this pull request Sep 21, 2020

QuantityFormatterTests: Formatting the 'expected' values using the CurrentUICulture #839

Merged

pgrawehr mentioned this pull request Nov 3, 2021

Default number format should be CultureInfo.CurrentCulture, not CurrentUICulture #787

Closed

pgrawehr mentioned this pull request Nov 13, 2021

Use CurrentCulture rather than CurrentUICulture #986

Merged

angularsen mentioned this pull request Nov 13, 2021

v5 Release #982

Merged

angularsen closed this Nov 14, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Use CurrentCulture rather than CurrentUICulture #795

Use CurrentCulture rather than CurrentUICulture #795

tmilnthorp commented May 26, 2020 •

edited by angularsen

Loading

codecov-commenter commented May 26, 2020 •

edited

Loading

angularsen commented May 27, 2020 •

edited

Loading

angularsen May 27, 2020 •

edited

Loading

tmilnthorp May 27, 2020

angularsen May 28, 2020

tmilnthorp May 28, 2020

tmilnthorp May 28, 2020

tmilnthorp May 29, 2020

pgrawehr May 29, 2020

angularsen May 29, 2020

lipchev May 29, 2020

pgrawehr May 29, 2020

tmilnthorp commented May 29, 2020

stale bot commented Jul 28, 2020

angularsen commented Nov 14, 2021

Use CurrentCulture rather than CurrentUICulture #795

Use CurrentCulture rather than CurrentUICulture #795

Conversation

tmilnthorp commented May 26, 2020 • edited by angularsen Loading

codecov-commenter commented May 26, 2020 • edited Loading

Codecov Report

angularsen commented May 27, 2020 • edited Loading

angularsen May 27, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tmilnthorp commented May 29, 2020

stale bot commented Jul 28, 2020

angularsen commented Nov 14, 2021

tmilnthorp commented May 26, 2020 •

edited by angularsen

Loading

codecov-commenter commented May 26, 2020 •

edited

Loading

angularsen commented May 27, 2020 •

edited

Loading

angularsen May 27, 2020 •

edited

Loading