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.

Sign up for GitHub

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

Jump to bottom

Port AllAnyToContainsRewritingExpressionVisitor to new pipeline #15806

Merged
merged 1 commit into from
May 26, 2019
Merged

Port AllAnyToContainsRewritingExpressionVisitor to new pipeline #15806

merged 1 commit into from
May 26, 2019

Conversation

roji
Copy link
Member

@roji roji commented May 25, 2019

Closes #15717

Functionality pretty close to the old implementation, but generating a Contains() MethodCallExpression directly (no need to go through SubQueryExpression as before).

Note TryExtractEqualityOperands() which could be a good candidate for a utility extension - this is probably a healthier/lower-risk approach than early normalization via a visitor.

@roji roji requested a review from smitpatel May 25, 2019 22:25
smitpatel
smitpatel reviewed May 25, 2019
View reviewed changes
src/EFCore/Query/Pipeline/AllAnyToContainsRewritingExpressionVisitor2.cs Outdated
negated = false;
if (mce.Arguments.Count == 1 && mce.Object?.Type == mce.Arguments[0].Type)
{
(left, right) = (mce.Object, mce.Arguments[0]);
Copy link
Contributor

Choose a reason for hiding this comment

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

Are we moving towards python like syntax?
While I am not opposed to this, we should define a consistent guideline as a team.

Copy link
Member Author

Choose a reason for hiding this comment

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

I've used this ever since the syntax was introduced, to me it makes a lot of sense precisely in cases like this where two separate lines just take up more space without readability value.

Not sure of the process for introducing new style :)

Copy link
Contributor

Choose a reason for hiding this comment

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

Just bring up in next meeting.

Copy link
Member

Choose a reason for hiding this comment

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

I've seen this a few places and like it, but agree we can discuss. At the same time we can talk about (shuddering to say it ) REGIONS!

Copy link
Member Author

Choose a reason for hiding this comment

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

I think I've seen some unauthorized regions lurking abut in @smitpatel's new pipeline code...

Copy link
Member

Choose a reason for hiding this comment

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

Yes, that's what I was referring to. ;-)

@roji roji merged commit b937ffa into master May 26, 2019
@ghost ghost deleted the AllAnyToContains branch May 26, 2019 00:44
@ajcvickers ajcvickers mentioned this pull request May 31, 2019
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@smitpatel smitpatel smitpatel approved these changes

@ajcvickers ajcvickers ajcvickers left review comments

@maumar maumar maumar approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Query: Port AllAnyToContainsRewritingExpressionVisitor
4 participants
@roji @ajcvickers @smitpatel @maumar