-
Notifications
You must be signed in to change notification settings - Fork 155
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Restructure AggregationBinder #421
base: main
Are you sure you want to change the base?
Restructure AggregationBinder #421
Conversation
src/Microsoft.AspNetCore.OData/Query/Expressions/AggregationBinderHelper.cs
Show resolved
Hide resolved
src/Microsoft.AspNetCore.OData/Query/Expressions/AggregationBinderHelper.cs
Outdated
Show resolved
Hide resolved
src/Microsoft.AspNetCore.OData/Query/Expressions/AggregationBinderHelper.cs
Outdated
Show resolved
Hide resolved
src/Microsoft.AspNetCore.OData/Query/Expressions/AggregationBinderHelper.cs
Outdated
Show resolved
Hide resolved
src/Microsoft.AspNetCore.OData/Query/Expressions/AggregationBinderHelper.cs
Show resolved
Hide resolved
ee97227
to
822bd21
Compare
src/Microsoft.AspNetCore.OData/Query/Expressions/AggregationBinder.cs
Outdated
Show resolved
Hide resolved
src/Microsoft.AspNetCore.OData/Query/Expressions/AggregationBinder.cs
Outdated
Show resolved
Hide resolved
src/Microsoft.AspNetCore.OData/Query/Expressions/AggregationBinder.cs
Outdated
Show resolved
Hide resolved
/// <summary> | ||
/// Helper class for <see cref="AggregationBinder"/>. | ||
/// </summary> | ||
internal class AggregationBinderHelper : QueryBinder |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think having an AggregationBinderHelper
class that inherits from QueryBinder
is good design. You can name this class AggregationBinderExtensions
and convert the methods to extension methods. Or move the method to BinderExtensions
class
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The FlattenReferencedProperties
method calls non-static methods in the QueryBinder
abstract class (Which is the base class for all query binder classes e.g FilterBinder
, SelectExpandBinder
).
We use the method inside the ApplyBind(this IAggregationBinder binder, IQueryable source, TransformationNode transformationNode, QueryBinderContext context, out Type resultClrType)
extension method which is static
.
/// <summary> | ||
/// Helper class for <see cref="AggregationBinder"/>. | ||
/// </summary> | ||
internal class AggregationBinderHelper : QueryBinder |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would probably understand if the class was named AggregationBinderBase
but I'd need to be convinced that inheritance is the way to go
The main changes:
AggregationBinder
properties inQueryBinderContext
AggregationBinder
implementIAggregationBinder
AggregationBinder
as an implementation ofIAggregationBinder
BindSelect
andBindGroupBy
methodsvirtual
.EFClassic
Make
AggregationBinder
public and extensible