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

Parse in operator before comparisons #524

Merged
merged 1 commit into from
Jul 10, 2021

Conversation

igitur
Copy link
Contributor

@igitur igitur commented Jun 22, 2021

Hi,

Inspired by SQL where clauses, I'm used to the in operator having precedence over others like and and or.

For example, I'm used to MainCompanyId in (1, 2) and Name in ("A") to be parsed as (((x.MainCompanyId == 1) OrElse (x.MainCompanyId == 2)) AndAlso (x.Name == "A")).

I think currently the parser attempts to parse it to something like ((((x.MainCompanyId == 1) OrElse (x.MainCompanyId == 2)) AndAlso x.Name) == "A"), which is invalid syntax.

A current workaround is to add additional parentheses and write the clause as (MainCompanyId in (1, 2)) and (Name in ("A")). This parses successfully, but it feels quite verbose to me.

This PR fixes the issue by first parsing for ins before comparisons. It was a simple fix and I'm almost surprised that it didn't break any existing tests (as far as I can see). I added a unit test for this PR's change.

Hope I didn't miss anything and that you agree with change in operator precedence in principle.

@StefH
Copy link
Collaborator

StefH commented Jul 2, 2021

@igitur
Thank you.

Can you also create a unit-test which really tests this functionality ? Maybe EntitiesTests ?

@StefH StefH added the bug label Jul 2, 2021
@igitur igitur force-pushed the in-operator-precedence branch from 20f2fb5 to 95b8c47 Compare July 7, 2021 10:20
@igitur
Copy link
Contributor Author

igitur commented Jul 7, 2021

Can you also create a unit-test which really tests this functionality ? Maybe EntitiesTests ?

Done. Added new partial class in EntitiesTests.In.cs.

Copy link
Collaborator

@StefH StefH left a comment

Choose a reason for hiding this comment

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

Thanks

@StefH StefH merged commit 7d59802 into zzzprojects:master Jul 10, 2021
@igitur igitur deleted the in-operator-precedence branch July 10, 2021 11:07
@igitur
Copy link
Contributor Author

igitur commented Aug 13, 2021

@StefH Any chance of a new release to include the changes?

@StefH
Copy link
Collaborator

StefH commented Aug 13, 2021

@igitur
It should already be included:
image

@igitur
Copy link
Contributor Author

igitur commented Aug 14, 2021

Oh, thanks!

Where do you see that?

I just kept looking at the Released entry on the repo's main page sidebar:
image

And then I assumed that my PR wasn't released yet.

@StefH
Copy link
Collaborator

StefH commented Aug 14, 2021

@StefH
Copy link
Collaborator

StefH commented Aug 14, 2021

I have to find a way to automatically convert a Tag into a Release here in GitHub...

@igitur
Copy link
Contributor Author

igitur commented Aug 14, 2021

https://github.com/zzzprojects/System.Linq.Dynamic.Core/blob/master/CHANGELOG.md

Thanks. I wasn't monitoring that.

I have to find a way to automatically convert a Tag into a Release here in GitHub...

Github already creates a draft release when you tag. To convert that to a final release isn't much extra effort. But I'm sure there will be some GitHub Actions script to do that.

@StefH
Copy link
Collaborator

StefH commented Aug 14, 2021

I found a github action which can do this:
https://github.com/softprops/action-gh-release

I've added this action on this project, so in the future, when I add a tag, this will be converted into a release.

@igitur
Copy link
Contributor Author

igitur commented Aug 14, 2021

Great, I might use that in my own projects.

@StefH
Copy link
Collaborator

StefH commented Aug 15, 2021

In case you are interested in generating the release notes in the same I do: take a look at https://github.com/StefH/GitHubReleaseNotes

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

Successfully merging this pull request may close these issues.

2 participants