CSHARP-1356: Fix Any workflow with Contains method. #13
Conversation
DmitryLukyanov
requested review from
craiggwilson,
jyemin,
rstam and
vincentkam
tests/MongoDB.Driver.Tests/Linq/Translators/PredicateTranslatorTests.cs
Outdated
Show resolved
Hide resolved
| @@ -163,8 +161,10 @@ private FilterDefinition<BsonDocument> TranslateAnd(BinaryExpression node) | |||
| return null; | |||
| } | |||
|
|
|||
| private bool CanAnyBeRenderedWithoutElemMatch(Expression node) | |||
| private bool CanAnyBeRenderedWithoutElemMatch(Expression node, out bool doNotUseExpressionParamInQueryGeneration) | |||
There was a problem hiding this comment.
having booleans that are negated is always difficult to think about. I believe it would be better for this to be useExpressionParamInQueryGeneration instead.
There was a problem hiding this comment.
Ok, I will rename this param
tests/MongoDB.Driver.Tests/Linq/Translators/PredicateTranslatorTests.cs
Outdated
Show resolved
Hide resolved
|
I amend my previous statement about the DocumentToFieldConverter... It is necessary and looks fine as it is. |
|
See proposed changes (with comments) here: |
DmitryLukyanov
force-pushed
the
CSHARP1356
branch
3 times, most recently
from
3b1611f
to
b86f222
Compare
There was a problem hiding this comment.
LGTM pending one comment where I asked @craiggwilson to LGTM the removal of the VisitPipeline method in the DocumentToFieldConverter class.
| protected internal override Expression VisitPipeline(PipelineExpression node) | ||
| { | ||
| return node; | ||
| } |
There was a problem hiding this comment.
@craiggwilson I would like you to LGTM that it is OK to remove this method.
There was a problem hiding this comment.
So, let's assume that I put it here for some reason (whatever that is). Is there any overriding reason to actually remove this other than "we don't know why it's here". If it's not causing issues, I don't see a reason to remove it just 'cause.
There was a problem hiding this comment.
If we don't remove it Dmitry's changes don't work.
So we should remove it, unless you can remember why you were suppressing visiting a PipelineExpression.
There was a problem hiding this comment.
Ok, if we have to remove it for things to work, then let's remove it.
DmitryLukyanov
force-pushed
the
CSHARP1356
branch
from
8899da8
to
d242d05
Compare
No description provided.