Make EvaluableExpressionFilter public #16776
Conversation
|
Also delete |
roji
force-pushed
the
MakeEvaluableFilterPublic
branch
from
724cb65
to
b35076b
Compare
|
Making a service public was more work than I thought... Also introduced dependency objects for both core and relational. |
There was a problem hiding this comment.
Deferring to @ajcvickers to finish review since there are dependencies now
| /// </summary> | ||
| public IModel Model { get; } | ||
| } | ||
| } |
There was a problem hiding this comment.
@roji Follow the same pattern as for all existing dependency objects and add validation test--see https://github.com/aspnet/EntityFrameworkCore/blob/master/test/EFCore.Tests/ModelSourceDependenciesTest.cs
There was a problem hiding this comment.
Added With method for Model and tests for both the core and relational dependencies (sorry, it's the first time I'm doing this).
Note that I added these because the API consistency tests told me too. I think there are various other cases of services without dependency objects (e.g. QueryContext accepts QueryContextDependencies, but RelationalQueryContext accepts additional parameters directly). We may want to do an extra pass for this.
There was a problem hiding this comment.
@roji Dependency objects are needed primarily when "outsiders" need to call constructors, possible from a derived class. This is expected anytime we have a public service implementation since, as you know from API review, we typically make these public when we expect people to inherit. So that's why there is a API consistency rule for those.
There are other places where we have added similar objects, but sometimes with a lighter-weight pattern (although maybe all our dependency objects could be read-only structs?). If there are places where you can see there will be problems evolving the API without dependency objects, then we should review those.
There was a problem hiding this comment.
OK, thanks for the explanation.
It seems a bit odd for a core base class (e.g. QueryContext) to have dependencies but not for its subclass in relational - if no outsider is expected to extend, then we don't need the deps in core, and if they do, then we do need them on relational, no?
Apart from that I don't yet have a good-enough feel for which services are expected to be extended (again, QueryContext?). It's probably not a big deal in any case.
There was a problem hiding this comment.
@roji Agreed that feels wrong. File an issue?
roji
force-pushed
the
MakeEvaluableFilterPublic
branch
from
b35076b
to
0d69d40
Compare
ghost
deleted the
No description provided.