Query: Remove public setters from custom expression properties #19346
Conversation
src/EFCore.Relational/Query/Internal/CaseWhenFlatteningExpressionVisitor.cs
Show resolved
Hide resolved
src/EFCore.Relational/Query/Internal/CaseWhenFlatteningExpressionVisitor.cs
Outdated
Show resolved
Hide resolved
src/EFCore.Relational/Query/Internal/CaseWhenFlatteningExpressionVisitor.cs
Show resolved
Hide resolved
| @@ -47,12 +47,25 @@ protected override Expression VisitExtension(Expression extensionExpression) | |||
| { | |||
| Check.NotNull(extensionExpression, nameof(extensionExpression)); | |||
|
|
|||
| return extensionExpression switch | |||
| #pragma warning disable IDE0066 // Convert switch statement to expression | |||
| switch (extensionExpression) | |||
There was a problem hiding this comment.
This should stay a switch expression... all the expressions are completely simple.
There was a problem hiding this comment.
First case is multi-line.
There was a problem hiding this comment.
Didn't we have this discussion already (or similar)? We have multi-line in conditional cases as well, which are also expressions (even nested multi-line)...
There was a problem hiding this comment.
Not to mention the ugliness of having to have a pragma around this...
There was a problem hiding this comment.
It does not look simple to me. I am not changing it.
There was a problem hiding this comment.
I do not consider this to be simple. The design meeting discussion we had specified guidelines to consider simple to be returning a specific value or calling into one method call. Nesting/Chaining is not something I consider simple.
There was a problem hiding this comment.
Also design meeting decision was not to use newer feature if something is not agreeable.
There was a problem hiding this comment.
I think we should discuss again with the team.
There was a problem hiding this comment.
Just extract the first case to a method, then it becomes simple enough
There was a problem hiding this comment.
I don't think we came to a consensus on this.
smitpatel
force-pushed
the
smit/cleanup19192
branch
from
dd4b466
to
0b33f11
Compare
smitpatel
changed the title
Resolves #19192 Changes - Only server side query expression are in place updated using private setters, public methods - Moved to Update method pattern in certain cases - TableExpressionBase still has internal set which will be fixed in #17337 - ShapedQueryExpression.VisitChildren throws now. Since ShapedQuery contains query expression (server side) and shaper expression (client side), it is unlikely that a visitor needs to visit both components. Hence we force visitor to decide which component to visit. This improves perf as we wouldn't go into client side shaper when updating something in sql tree
smitpatel
force-pushed
the
smit/cleanup19192
branch
from
0b33f11
to
c41a2f1
Compare
Resolves #19192
Changes