Decimal separator different in different windows language pack #111
Conversation
|
I can, but see my reasoning at #74 (comment) if the fix should be more than just comparing in the tests. I.e. if the files have other use in diffing and just making sure the codebase would be consistent (I see it's a bit of a problem the printing logic is spread so one has to apply |
| @@ -615,7 +616,8 @@ protected bool CheckEqualityFromPathsCore(string relPath, string basePath, strin | |||
|
|
|||
| private static void GetNumbersFromFile(ref string firstString, ref string secondString, decimal precision) | |||
| { | |||
| Regex _matchNumer = new Regex(@"\b[0-9]+\.?[0-9]*\b", RegexOptions.IgnoreCase | RegexOptions.Compiled); | |||
| string decimalSeparator = CultureInfo.CurrentCulture.NumberFormat.NumberDecimalSeparator; | |||
| Regex _matchNumer = new Regex(@"\b[0-9]+\" + decimalSeparator + "?[0-9]*\b", RegexOptions.IgnoreCase | RegexOptions.Compiled); | |||
There was a problem hiding this comment.
Does a , need a backslash before it like a decimal point . does? I see you left the backslash in.
There was a problem hiding this comment.
For , the backslash wont have any effect on the output of the regular expression.
| @@ -615,7 +616,8 @@ protected bool CheckEqualityFromPathsCore(string relPath, string basePath, strin | |||
|
|
|||
| private static void GetNumbersFromFile(ref string firstString, ref string secondString, decimal precision) | |||
| { | |||
| Regex _matchNumer = new Regex(@"\b[0-9]+\.?[0-9]*\b", RegexOptions.IgnoreCase | RegexOptions.Compiled); | |||
| string decimalSeparator = CultureInfo.CurrentCulture.NumberFormat.NumberDecimalSeparator; | |||
| Regex _matchNumer = new Regex(@"\b[0-9]+\" + decimalSeparator + "?[0-9]*\b", RegexOptions.IgnoreCase | RegexOptions.Compiled); | |||
There was a problem hiding this comment.
Don't you need to check for both characters? The secondString will have the current culture's separator in it. But the firstString will come from the baseline file, which probably has the InvariantCulture's separator in it (i.e. .).
| @@ -615,7 +615,7 @@ protected bool CheckEqualityFromPathsCore(string relPath, string basePath, strin | |||
|
|
|||
| private static void GetNumbersFromFile(ref string firstString, ref string secondString, decimal precision) | |||
| { | |||
| Regex _matchNumer = new Regex(@"\b[0-9]+\.?[0-9]*\b", RegexOptions.IgnoreCase | RegexOptions.Compiled); | |||
| Regex _matchNumer = new Regex(@"\b[0-9]+[\.,]?[0-9]*\b", RegexOptions.IgnoreCase | RegexOptions.Compiled); | |||
There was a problem hiding this comment.
Are there any other decimal separators in other languages that we should care about?. I think using CurrentCulture.NumberFormatInfo was a good idea.
|
Hi all, should we not instead fix this issue by just always running tests in one culture, possibly invariant? I'd rather just run the tests consistently than have to constantly worry about having to be sure I always caught every conceivable issue with every culture. Edit: Actually I think @veikkoeeva is probably fixing the main issue here in PR 109. I'd prefer not to fix things the same way as is done in this PR because the more "flexible" we make the regex the more likely it is that it will have some false detections; we do have some situations in yet-to-be-imported baselines, I'm fairly certain, where we separate multiple numbers using commas. :( |
ghost
locked as resolved and limited conversation to collaborators
Fixes #74
The decimal separators are different for different language packs eg "," is a decimal separator for spanish. the output written to these files will include comma instead of decimal as in our zbaseline files.
The problem could be avoided by improving our number regular expression to use decimal separator dynamically which will allow us to pick these values as numbers and match them as numbers rather than strings
@veikkoeeva can you please test this pr on your machine. I currently dont have access to the non-english windows
cc @eerhardt @danmosemsft @codemzs