Regex parsing #64
Regex parsing #64
Conversation
|
|
||
| var numFormat = formatProvider != null ? | ||
| (NumberFormatInfo) formatProvider.GetFormat(typeof (NumberFormatInfo)) : | ||
| (NumberFormatInfo) CultureInfo.CurrentCulture.NumberFormat.Clone(); |
There was a problem hiding this comment.
I don't know if you considered CurrentCulture vs CurrentUICulture here? I'm not sure which one is right.
http://stackoverflow.com/a/329041/134761
CurrentCulture is the .NET representation of the default user locale of the system. This controls default number and date formatting and the like.
CurrentUICulture refers to the default user interface language, a setting introduced in Windows 2000. This is primarily regarding the UI localization/translation part of your app.
I've used CurrentUICulture as default in the unit classes' ToString() and in UnitSystem for outputting and parsing unit abbreviations, since I figured that was the most closely related to localization and text presentation in an app.
I don't know if CurrentCulture or CurrentUICulture is best suited for parsing quantities in this context? Normally the two are similar, but there is a reason they are separate. The scenario I can best relate to is if an app allows me to choose language, then I usually choose English. I haven't done much localization myself, but I assume it then sets the Thread.CurrentUICulture to "en-US" so those text resources are loaded etc. But being Norwegian, I prefer reading Norwegian dates and numbers so I have configured my Windows to show that. So I'm thinking Thread.CurrentCulture might remain "nb-NO" in a properly localized app, so it shows and parses numbers and dates in my preferred format. If that is the case, then CurrentCulture should be used in this line.
Thoughts?
Sorry for the big post, but I'm fuzzy on what's best practice here.
There was a problem hiding this comment.
I am actually not sure about this myself, but I think we should look at what double.Parse uses by default (considering the quantity is just forwarded there). Looking at the docs it looks like NumberFormatInfo.CurrentInfo is used which is essentially using CultureInfo.CurrentCulture.NumberFormat.
Do you think it would be better to switch to NumberFormatInfo.CurrentInfo?
There was a problem hiding this comment.
Then we agree. It was probably not clear from my post, but after the train of thoughts I wound up with CurrentCulture too. That would also support the scenario I described, which is neat.
There was a problem hiding this comment.
CurrentInfo might be an improvement, I like it. Sort of makes it clear what the intention is.
|
Awesome work, huge improvement! Look forward to your comments on my input. |
|
Hope I didn't hog your github stats in any way by creating the pull request for you. Create your own PR if you want. |
|
I'm surprised that I didn't get a notification about the pull request. Just decided to take a look here after I saw your email. Also, I'm not concerned with the github stats :) This way is good too. I didn't create the pull request intially in case you had any major comments, like if you were already working on something similar (maybe on a local branch), or had any reservations about this approach. I will not hesitate to create it in the future :) |
…s, and minor corrections
…ult parsing culture
|
7b0a1f0 updated the format strings as suggested. Is there anything that still needs to be done on the parsing? |
|
I'll take a look at this tomorrow night.
|
|
Merged in to develop on de05aad. |
|
Nuget 3.7.1 is out. |
No description provided.