issue #71 #104
issue #71 #104
Conversation
|
Please to merge this PR |
|
@koenbeuk Please merge this PR |
koenbeuk
left a comment
koenbeuk
left a comment
There was a problem hiding this comment.
Thanks for this PR, Apart from some minor changes, I did propose a workaround for the added unit test. functionality that would work today. Can you let me know in what scenarios my proposed workaround would not support and why we would need this PR merged?
Thanks again!
| { | ||
| public ProjectableAttribute() { } | ||
|
|
||
| public ProjectableAttribute(string useMemberBody, object memberBodyParameterValue) |
There was a problem hiding this comment.
A constructor would be an aggressive change as this would limit our ability in the future to add additional constructor arguments. Just having a property would be sufficient. Can this constructor be dropped?
| /// <summary> | ||
| /// Parameters values for UseMemberBody. | ||
| /// </summary> | ||
| public object[]? MemberBodyParameterValues { get; set; } |
There was a problem hiding this comment.
I would prefer the name UseMemberBodyArguments
| if (reflectedExpression.Parameters.Count > 1) | ||
| { | ||
| var projectableAttribute = nodeMember.GetCustomAttribute<ProjectableAttribute>(false)!; | ||
| foreach (var prm in reflectedExpression.Parameters.Skip(1).Select((Parameter, Index) => new { Parameter, Index })) |
There was a problem hiding this comment.
I would prefer calling this parameterWithIndex to stay in sync with the naming conventions used throughout the rest of the code base
| var projectableAttribute = projectableMemberInfo.GetCustomAttribute<ProjectableAttribute>() | ||
| ?? throw new InvalidOperationException("Expected member to have a Projectable attribute. None found"); | ||
|
|
||
| var expression = GetExpressionFromGeneratedType(projectableMemberInfo); |
There was a problem hiding this comment.
There should be no need to call this if projectableAttribute.UseMemberBody is not null
|
|
||
| if (expression is null && projectableAttribute.UseMemberBody is not null && projectableAttribute.MemberBodyParameterValues is null) | ||
| { | ||
| expression = GetExpressionFromGeneratedType(projectableMemberInfo, true, projectableAttribute.UseMemberBody); |
There was a problem hiding this comment.
The result will be overwritten by the call on line 24
There was a problem hiding this comment.
I think differently that when calling line 20 the expression will not be empty
| public int Id { get; set; } | ||
| public List<OrderItem> OrderItem { get; set; } = new List<OrderItem>(); | ||
|
|
||
| [Projectable(nameof(GetOrderItemNameExpression), 1)] |
There was a problem hiding this comment.
This example confuses me, could you not just have done:
[Projectable(UseMemberBody = nameof(GetOrderItemNameInnerExpressionWithArgument))]
public string FirstOrderPropName => GetOrderItemName(1);
private static Expression<Func<Order, string>> GetOrderItemNameInnerExpressionWithArgument()
=> new MyParameterExpressionVisitor(parameter: "id", value: 1).Visit(GetOrderItemNameInnerExpression());
private static Expression<Func<Order, int, string>> GetOrderItemNameInnerExpression()
=> (@this, id) => @this.OrderItem
.Where(av => av.Id == id)
.Select(av => av.Name)
.FirstOrDefault() ?? string.Empty;Where MyParameterExpressionVisitor swaps out the parameter named id for a constant value.
There was a problem hiding this comment.
I'm also confused, but we generate this call in runtime and therefore it is convenient for us to pass the value of the parameters through the attribute.
|
Hi, @koenbeuk When you planning to merge this PR? |
3 participants
This request contains a solution to problem #71 and a test case of use. The request was prepared with the support of P9avel.