-
Notifications
You must be signed in to change notification settings - Fork 95
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
Native currency names #49
Conversation
What kind of issue did you get? This looks wrong. |
The issue is that the call to CultureInfo.GetCulture returns cultures in different order on different machines. In the function InitCurrencyCodeTable this introduces a problem when distinct currencies are retrieved. private static void InitCurrencyCodeTable()
{
lock (Obj)
{
bool valid(CultureInfo c) => !c.IsNeutralCulture && !c.EnglishName.StartsWith("Unknown Locale") && !c.EnglishName.StartsWith("Invariant Language");
CurrencyTable = CultureInfo.GetCultures(CultureTypes.AllCultures)
.Where(valid)
.Select(c => new Currency(c)).Cast<ICurrency>()
.Distinct(new CurrencyEqualityComparer())
.ToDictionary(k => k.CurrencyIsoCode, e => e);
CurrencyTable.Add("BTC", new Currency("BitCoin", "₿", "BTC", 8));
}
} The code block below returns only the cultures that have SEK as currency. On different machines (with the same user and CurrentCulture "sv-SE") this is returned in different order. var cultures = CultureInfo.GetCultures(CultureTypes.AllCultures)
.Where(valid)
.Select(c => new Currency(c))
.Where(c => c.CurrencyIsoCode == "SEK")
.ToDictionary(k => k.Culture, e => e); The currency is the same, but the native translation is different based on the culture. Will provide a new commit where the CurrentCulture currency is always used regardless of retrieval order. |
6d68a17
to
ae68032
Compare
Still unclear why this is a problem. Furthermore, the proposed fix also looks arbitrary and unreliable. |
Read both native and english currency name from region settings. Make currency from system culture the default if no default is configured.
27ebd00
to
46818fe
Compare
The problem for me was that I couldn't find Swedish Krona and configure it as default currency. The first loaded culture has a native name that I don't understand for the currency SEK. Made some more changes to make it more reliable and should now offer a good startup experience in most regions.
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Making the native name clear sounds good, but I think the default needs to be addressed differently.
MoneyDataType/KnownCurrencyTable.cs
Outdated
CurrencyTable.Remove(currentCultureCurrency.CurrencyIsoCode); | ||
CurrencyTable.Add(currentCultureCurrency.CurrencyIsoCode, currentCultureCurrency); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I still don't understand how this works. Why would you replace this in a static method? You can't know that what is the current culture when this runs (which is pretty much non-deterministic) is going to remain the current culture. The current culture potentially changes all the time. The whole idea of defaulting to the current culture sounds good on the surface, but I don't think it holds water. The default culture of the site may be a better choice, but even if you do that, you would certainly not modify the table (especially not in a static method: all tenants share that if I'm not mistaken), you'd instead select the correct current culture when it's queried for.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
NativeName is currently never shown so this change is rolled back.
Services/MoneyService.cs
Outdated
@@ -35,7 +36,7 @@ public ICurrency DefaultCurrency | |||
{ | |||
var defaultIsoCode = _options?.DefaultCurrency; | |||
return string.IsNullOrEmpty(defaultIsoCode) | |||
? Currency.USDollar | |||
? new Currency(CultureInfo.CurrentCulture) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
See my other comment.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree with this. Have rolled back this change.
9f413bd
to
617ff43
Compare
Thanks for the contribution! |
Changed to use the english currency name. We got random results with the name on different machines.