-
Notifications
You must be signed in to change notification settings - Fork 457
Parse double precision using InvariantCulture #2722
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
base: main
Are you sure you want to change the base?
Parse double precision using InvariantCulture #2722
Conversation
@@ -362,7 +362,7 @@ bool ParsePrimary() | |||
if (ParsePattern(EvaluateNumberPattern())) | |||
{ | |||
string _number = PatternMatch.Groups[1].Value; | |||
RPN.Add(new MathToken(MathTokenType.Value, _number, double.Parse(_number))); | |||
RPN.Add(new MathToken(MathTokenType.Value, _number, double.Parse(_number, System.Globalization.CultureInfo.InvariantCulture))); |
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.
Each converter provides CultureInfo. Maybe it's better to pass it here?
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.
This prevents the ability to use a comma as a decimal separator too.
Perhaps we can make it controllable via a property on the converter instead?
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.
The ParsePattern(EvaluateNumberPattern())
has a regex in it that pretty much assumes US english. The double.Parse() is merely matching it.
If we change it so that it reacts to a passed-in CultureInfo either via the converter (or equivalent), then, I would need to supply a regex for it, else rework how the numbers are being extracted from the string.
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.
@VladislavAntonyuk I tried passing a custom CultureInfo
to the converter via XAML but couldn’t find the syntax. It seems the converter uses CultureInfo.CurrentUICulture
, which brings us back to the issue, i.e. expressions can fail in certain cultures.
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.
@VladislavAntonyuk @bijington InvariantCulture
appears to be used consistently for numeric values in XAML and for numeric expressions in C#. Based on that, I still believe InvariantCulture
is the appropriate choice for MathExpressionConverter
. After further research, CurrentCulture
is generally reserved for UI-facing operations, which doesn't apply to the kind of expressions handled by this converter.
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.
Thanks for the explanation. That's fine with me 👍
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.
Does that mean that I can't use comma as a separator?
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.
@VladislavAntonyuk, the analogy would be to compare it with another double precision XAML property, e.g. VisualElement.Opacity. When one sets that in XAML, don't they also specify that double using InvariantCulture? e.g.
Opacity=".5" |
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.
@VladislavAntonyuk, are you okay with this explanation? I will assume this discussion is resolved.
@@ -362,7 +362,7 @@ bool ParsePrimary() | |||
if (ParsePattern(EvaluateNumberPattern())) |
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.
It might be good to turn on the CA1305 analyzer in the project to avoid similar bugs.
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.
@MartyIX based on your feedback, I applied the CA1305 analyzer and found multiple conversions in MathExpression.shared.cs that needed updates to satisfy the analyzer. After the changes all unit tests for the math converters still passed.
The CA1305 analyzer also picked up new issues which isn't part of the scope of this PR. Would you like me to create split 3 issues to look? (Camera CA1305, SpeechToText CA1305, Badge CA1305)?
F:\Maui\src\CommunityToolkit.Maui.Camera\CameraInfo.shared.cs(99,4): error CA1305: The behavior of 'StringBuilder.Append(ref StringBuilder.AppendInterpolatedStringHandler)' could vary based on the current user's locale settings. Replace this call in 'CameraInfo.ToString()' with a call to 'StringBuilder.Append(IFormatProvider, ref StringBuilder.AppendInterpolatedStringHandler)'. (https://learn.microsoft.com/dotnet/fundamentals/code-analysis/quality-rules/ca1305)
F:\Maui\src\CommunityToolkit.Maui.Camera\CameraInfo.shared.cs(99,4): error CA1305: The behavior of 'StringBuilder.Append(ref StringBuilder.AppendInterpolatedStringHandler)' could vary based on the current user's locale settings. Replace this call in 'CameraInfo.ToString()' with a call to 'StringBuilder.Append(IFormatProvider, ref StringBuilder.AppendInterpolatedStringHandler)'. (https://learn.microsoft.com/dotnet/fundamentals/code-analysis/quality-rules/ca1305)
F:\Maui\src\CommunityToolkit.Maui.Camera\CameraInfo.shared.cs(99,4): error CA1305: The behavior of 'StringBuilder.Append(ref StringBuilder.AppendInterpolatedStringHandler)' could vary based on the current user's locale settings. Replace this call in 'CameraInfo.ToString()' with a call to 'StringBuilder.Append(IFormatProvider, ref StringBuilder.AppendInterpolatedStringHandler)'. (https://learn.microsoft.com/dotnet/fundamentals/code-analysis/quality-rules/ca1305)
F:\Maui\src\CommunityToolkit.Maui.Camera\CameraInfo.shared.cs(99,4): error CA1305: The behavior of 'StringBuilder.Append(ref StringBuilder.AppendInterpolatedStringHandler)' could vary based on the current user's locale settings. Replace this call in 'CameraInfo.ToString()' with a call to 'StringBuilder.Append(IFormatProvider, ref StringBuilder.AppendInterpolatedStringHandler)'. (https://learn.microsoft.com/dotnet/fundamentals/code-analysis/quality-rules/ca1305)
F:\Maui\src\CommunityToolkit.Maui.Camera\CameraInfo.shared.cs(99,4): error CA1305: The behavior of 'StringBuilder.Append(ref StringBuilder.AppendInterpolatedStringHandler)' could vary based on the current user's locale settings. Replace this call in 'CameraInfo.ToString()' with a call to 'StringBuilder.Append(IFormatProvider, ref StringBuilder.AppendInterpolatedStringHandler)'. (https://learn.microsoft.com/dotnet/fundamentals/code-analysis/quality-rules/ca1305)
F:\Maui\src\CommunityToolkit.Maui.Core\Essentials\SpeechToText\OfflineSpeechToTextImplementation.android.cs(188,56): error CA1305: The behavior of 'int.ToString()' could vary based on the current user's locale settings. Replace this call in 'RecognitionSupportCallback.OnError(int)' with a call to 'int.ToString(IFormatProvider)'. (https://learn.microsoft.com/dotnet/fundamentals/code-analysis/quality-rules/ca1305)
F:\Maui\src\CommunityToolkit.Maui.Core\Essentials\Badge\BadgeImplementation.windows.cs(22,40): error CA1305: The behavior of 'uint.ToString()' could vary based on the current user's locale settings. Replace this call in 'BadgeImplementation.SetCount(uint)' with a call to 'uint.ToString(IFormatProvider)'. (https://learn.microsoft.com/dotnet/fundamentals/code-analysis/quality-rules/ca1305)
Build failed with 7 error(s) and 81 warning(s) in 3.8s
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.
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.
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.
You can change the pattern to something like this:
@"^-?\d+[.,]?\d*$"
It should match:
- 123
- -123.45
- -123,45
Description of Change
Parse a double using InvariantCulture to avoid issues when CurrentCulture changes, such as to 'ar-AR'.
Linked Issues
PR Checklist
approved
(bug) orChampioned
(feature/proposal)main
at time of PRAdditional information
None