-
Notifications
You must be signed in to change notification settings - Fork 3.2k
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
Don't duplicate query filter parameters #29422
Conversation
If there's anything I need to do to move this along, let me know. If it's just that other things are taking a priority, that's fine! |
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.
Bad implementation
src/EFCore/Query/Internal/ParameterExtractingExpressionVisitor.cs
Outdated
Show resolved
Hide resolved
6bc8917
to
10e4d31
Compare
Is the failed build an environmental issue? Looks like it cancelled after waiting 3 hours. Not sure if it can be retried without another push. Edit: failed again, so might be due to a change in my PR after all. All builds ok locally so a bit stumped. |
@smitpatel thanks for initial review, changes are much simpler now. |
@maumar this post here may be useful for understanding how I arrived at these changes: #29422 (comment) - thanks |
@maumar any feedback? |
@ajcvickers is this likely to get reviewed again anytime soon? After @smitpatel's feedback, the changes are quite small and I'm fairly confident in them. |
@stevendarby We will get to it when we can, but since Smit is no longer on the team it is taking some time. |
@ajcvickers ah I see, thanks for the update |
@maumar sorry to pester, but now the 8.0 previews have begun I was just thinking it'd be great to get it into one; and just aware that another 5 months will be too late. Is there any chance of a review over the next few weeks? |
@stevendarby I'm planning to take a look at it this week. Apologies for the long delay. |
src/EFCore/Query/Internal/ParameterExtractingExpressionVisitor.cs
Outdated
Show resolved
Hide resolved
554bc7d
to
af1b68e
Compare
src/EFCore/Query/Internal/NavigationExpandingExpressionVisitor.cs
Outdated
Show resolved
Hide resolved
af1b68e
to
0865ece
Compare
@stevendarby thank you for the contribution! |
Fixes #24476