Refactor & optimize ProjectionExpression, TranslatedQuery (#40)#181
Conversation
* Refactor & optimize ProjectionExpression, TranslatedQuery * EOL at EOF * Fix Translator.TranslateMethod * Get rid of Translator.TranslateMethod because we need not it for non-generic method * Rename ProjectionExpression.Select() -> .Apply() * Avoid list allocation when calling Translator.Translate()
| // Creates new ItemProjectorExpression, based on this but with new ItemProjectorExpression | ||
| public ProjectionExpression Apply(ItemProjectorExpression itemProjectorExpression) => | ||
| new ProjectionExpression(Type, itemProjectorExpression, TupleParameterBindings, ResultAccessMethod); |
There was a problem hiding this comment.
Within translation we have ApplyProvider (also CompilableProviderExtensions.Apply), ApplyParameter, so I worry that this new Apply might be misunderstood while reading. Can we think of another name, like ApplyItemProjector or something else but kind of more specific?
What's the point of the comment if we can see it only when we look at the method itself and what the method does is pretty clear, btw it's misleading because it creates new ProjectionExpression with new item projector.
There was a problem hiding this comment.
We had a discussion about this naming with @AlexUstinov
Here is his argument:
What do you think about renaming this to Apply?
@SergeiPavlov SergeiPavlov 26 days ago Author Member
The better name by Category theory would be Project(). We are projecting, applying projecter.
In LINQ .Select() makes a projection, that is why I named it Select
Apply seems to be very general word, any operation is application some function to arguments.
@AlexUstinov AlexUstinov 23 days ago Member
Select on the other hand is firmly associated with LINQ, so it adds a considerable level of confusion.
When I suggested Apply I thought about the typical use case
var newProjectionExpression = ProjectionExpression.Select(newItemProjector);
when one expression is being transformed to a new one by the instance of a Projector.
So there are two options here if we want to replace the select with something.
We either Apply (or it can be another synonym name like TransformWith) a Projector to the current expression instance as I suggested.
var newProjectionExpression = ProjectionExpression.Apply(newItemProjector);
or alternatively we can use a Projector to transform the expression and in that case the Project name you mentioned works just fine.
var newProjectionExpression = newItemProjector.Project(ProjectionExpression);
There was a problem hiding this comment.
What's the point of the comment if we can see it only when we look at the method itself and what the method does is pretty clear, btw it's misleading because it creates new ProjectionExpression with new item projector.
Removed the comment
There was a problem hiding this comment.
Basically, @AlexUstinov brought the same reason for Select - it misleads the reader. I don't know whether he knows that 'Apply' has certain meaning during translation as well as 'Select' in LINQ.
In general, I agree with .Apply() because it fits the action, we have a lot of .Apply() methods outside translation, you can find them easily. But in translation we have dataSource.Apply(...) to wrap current datasouce with ApplyProvider and projection.Apply(...) that substitutes ItemProjector, which could be misleading
|
@SergeiPavlov , please resolve merge conflicts and will merge the PR. |
|
Resolved |
|
Thank you |
3 participants
ProjectExpression.Apply()method