Skip to content
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

TypeHelper#ParseNumber TryParse does not use InvariantCulture #320

Closed
mvalero opened this issue Nov 13, 2019 · 6 comments
Closed

TypeHelper#ParseNumber TryParse does not use InvariantCulture #320

mvalero opened this issue Nov 13, 2019 · 6 comments
Labels

Comments

@mvalero
Copy link

mvalero commented Nov 13, 2019

When parsing a dynamic expression that contains a real literal like 12.02 or 1.0, the resulting LambdaExpression works differently depending on the machine's configured locale.

i.e:
Following literal expression should be parsed to a decimal:

var l = DynamicExpressionParser.ParseLambda(false,
   new ParameterExpression[] {}, typeof(decimal), "3.21", new object[] {});

But in my machine (with a german locale) the resulting expression gets the value 321 instead of 3.21 ->
image

Trying the same in a machine with a US locale it works as expected.

When looking into the TypeHelper#ParseNumber method:
https://github.com/StefH/System.Linq.Dynamic.Core/blob/master/src/System.Linq.Dynamic.Core/Parser/TypeHelper.cs#L498

you can see that decimal.TryParse(text, out e) gets called without using the InvariantCulture which leads to different results depending on the locale of the machine where the code is being executed.

When calling ParseLambda with 3.21m the expression get's parsed correctly.

I'm not sure if that's the expected behavior, but in my opinion both cases should work independently of the locale as the literal in an expression normaly would use . as a decimal separator: https://github.com/kahanu/System.Linq.Dynamic/wiki/Dynamic-Expressions#literals

@StefH
Copy link
Collaborator

StefH commented Nov 13, 2019

Good point.
However I think that this behavior should be configurable. So better to add NumberStyles and/or CultureInfo to ParsingConfig.cs

@StefH StefH closed this as completed Nov 13, 2019
@StefH StefH reopened this Nov 13, 2019
@mvalero
Copy link
Author

mvalero commented Nov 13, 2019

Having it configurable would be a nice addition, the default should still be InvariantCulture in my opinion to avoid unexpected behaviour.

@StefH
Copy link
Collaborator

StefH commented Nov 20, 2019

@mvalero
I've created a PR, you can review this --> #323

And if you want to test, you can use the System.Linq.Dynamic.Core.1.0.20-ci-12197.nupkg from MyGet.

@StefH StefH added the bug label Nov 22, 2019
@StefH
Copy link
Collaborator

StefH commented Dec 13, 2019

@mvalero Can you please test version System.Linq.Dynamic.Core.1.0.20-ci-12197 from MyGet ?

@mvalero
Copy link
Author

mvalero commented Dec 16, 2019

@StefH Sorry for the delayed response. Looks good, I just tested the version from MyGet and it solves the problem I described.

Thx a lot.

@StefH
Copy link
Collaborator

StefH commented Dec 16, 2019

@mvalero Thank you very much for verifying.

@StefH StefH closed this as completed Dec 16, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Development

No branches or pull requests

2 participants