CSHARP-5551: Support KeyValuePair.Create<TKey,TValue>method in LINQ #1661
Conversation
sanych-sun
requested review from
JamesKovacs,
rstam and
adelinowona
and removed request for
a team and
JamesKovacs
| #if NET6_0_OR_GREATER || NETSTANDARD2_1_OR_GREATER | ||
| if (method.Is(KeyValuePairMethod.Create)) | ||
| { | ||
| return NewKeyValuePairExpressionToAggregationExpressionTranslator.TranslateKeyValuePairExpression(context, expression, expression.Arguments); |
There was a problem hiding this comment.
I would break apart the arguments and pass them individually for perfect clarity:
var keyExpression = arguments[0];
var valueExpression = arguments[1];
return NewKeyValuePairExpressionToAggregationExpressionTranslator.Translate(context, expression, keyExpression, valueExpression);
| => expression.Type.IsConstructedGenericType && expression.Type.GetGenericTypeDefinition() == typeof(KeyValuePair<,>); | ||
|
|
||
| public static TranslatedExpression Translate(TranslationContext context, NewExpression expression) | ||
| => TranslateKeyValuePairExpression(context, expression, expression.Arguments); |
There was a problem hiding this comment.
I would break apart the arguments and pass them individually for perfect clarity:
public static TranslatedExpression Translate(TranslationContext context, NewExpression expression)
{
var arguments = expression.Arguments;
var keyExpression = arguments[0];
var valueExpression = arguments[1];
return Translate(context, expression, keyExpression, valueExpression);
}
I realize there is a little bit of duplication but I think the clarity of giving the keyExpression and valueExpression meaningful variable names is worth it.
|
|
||
| public static TranslatedExpression TranslateKeyValuePairExpression(TranslationContext context, Expression expression, IReadOnlyList<Expression> arguments) | ||
| { | ||
| var translatedKeyExpression = ExpressionToAggregationExpressionTranslator.Translate(context, arguments[0]); |
There was a problem hiding this comment.
The name we usually use for this variable is keyTranslation.
| public static TranslatedExpression Translate(TranslationContext context, NewExpression expression) | ||
| => TranslateKeyValuePairExpression(context, expression, expression.Arguments); | ||
|
|
||
| public static TranslatedExpression TranslateKeyValuePairExpression(TranslationContext context, Expression expression, IReadOnlyList<Expression> arguments) |
There was a problem hiding this comment.
Instead of having an arguments parameter use explicit keyExpression and valueExpression parameters:
public static TranslatedExpression Translate(
TranslationContext context,
Expression expression,
Expression keyExpression,
Expression valueExpression)
The name can be shortened to Translate. There is no ambiguity with this signature.
| classMap.GetMemberMap("Value").SetSerializer(valueSerializer); | ||
| classMap.Freeze(); | ||
|
|
||
| // have to use BsonClassMapSerializer here to mimic the MemberInitExpressionToAggregationExpressionTranslator to avoid behavioral breaking change |
There was a problem hiding this comment.
Maybe reword "avoid behavioral breaking change" to "avoid risking a behavioral breaking change"
We don't think there is a behavioral change, we just want to play it safe.
| public static TranslatedExpression TranslateKeyValuePairExpression(TranslationContext context, Expression expression, IReadOnlyList<Expression> arguments) | ||
| { | ||
| var translatedKeyExpression = ExpressionToAggregationExpressionTranslator.Translate(context, arguments[0]); | ||
| var translatedValueExpression = ExpressionToAggregationExpressionTranslator.Translate(context, arguments[1]); |
There was a problem hiding this comment.
The name we usually use for this variable is valueTranslation.
| return Translate(context, expression, keyExpression, valueExpression); | ||
| } | ||
|
|
||
| public static TranslatedExpression Translate(TranslationContext context, Expression expression, Expression keyExpression, Expression valueExpression) |
There was a problem hiding this comment.
By the time the line gets this long I start putting the parameters on separate lines:
public static TranslatedExpression Translate(
TranslationContext context,
Expression expression,
Expression keyExpression,
Expression valueExpression)
No description provided.