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

Use parameterized names in dynamic query #399

Conversation

ascott18
Copy link
Contributor

Builds on #331 - added wrapping of constants as soon as they're parsed to avoid cases where they can slip through the cracks.

@@ -737,6 +737,8 @@ Expression ParseUnary()
Expression ParsePrimary()
{
Expression expr = ParsePrimaryStart();
_expressionHelper.WrapConstantExpression(ref expr);
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@StefH This is the principal change here. I'll let you determine if this makes sense to fix the issue in this way or not.


[Theory]
[InlineData("NullableIntValue", "42")]
[InlineData("NullableDoubleValue", "42.23")]
Copy link
Contributor Author

@ascott18 ascott18 Jul 10, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

NullableDoubleValue is the test case for the issue in #331 (comment)

@ascott18
Copy link
Contributor Author

@JonathanMagnan any updates on this? Would love to see this merged (or rejected, if that's the right answer).

@JonathanMagnan
Copy link
Member

Hello @ascott18 ,

Sorry for the delay,

I'm currently looking at it.

There is currently some weird stuff such as why a double constant require an Expression.Convert to be of type Nullable<double> while an int doesn't require such Convert. We already know where this is happening but we need to look deeper if such code is really required as you normally try to avoid Convert when not needed.

I'm not sure either if this line https://github.com/zzzprojects/System.Linq.Dynamic.Core/pull/399/files#diff-e02763e459f8622732aa03abaf297810R740 should really exists in the ParsePrimary or not.

I will continue to look at it and keep you updated later this week.

@JonathanMagnan JonathanMagnan merged commit 02910bd into zzzprojects:master Aug 7, 2020
JonathanMagnan added a commit that referenced this pull request Aug 7, 2020
Fix unit test project caused by pull #399 and allow parse double
@JonathanMagnan
Copy link
Member

Hello @ascott18 ,

Your pull has been merged in the v1.2.1

Thank a lot for your help and your follow-up ;)

Best Regards,

Jon

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants