QueryBuilder does not need generics. #18133
Conversation
jpountz
added
>enhancement
:Core/Infra/Transport API
| @@ -50,7 +50,7 @@ | |||
| /** | |||
| * Sets the arbitrary name to be assigned to the query (see named queries). | |||
| */ | |||
| QB queryName(String queryName); | |||
| QueryBuilder queryName(String queryName); | |||
There was a problem hiding this comment.
This is why it had the generics. OTOH I'll bet if we always return the subclass of QueryBuilder anyplace interesting and have each of them implement this method by returning their own type then no client code will have to change. We should probably add something to the docs on this method telling implementers to return their own type, not QueryBuilder.
There was a problem hiding this comment.
It works fine in practice because query builders are always constructed with their concrete type. With this pull request already, none of the client code that we had in the test folders had to change.
I will add the note to the javadocs of QueryBuilder (pointing out that extending AbstractQueryBuilder does it for you).
There was a problem hiding this comment.
extending AbstractQueryBuilder does it for you
Awesome!
There was a problem hiding this comment.
I just pushed one more commit with docs.
|
I felt a great disturbance in the Force, as if hundreds of spurious warnings suddenly cried out in terror, and were suddenly silenced. I fear something awesome has happened. |
|
LGTM |
QueryBuilder has generics, but those are never used: all call sites use `QueryBuilder<?>`. Only `AbstractQueryBuilder` needs generics so that the base class can contain a default implementation for setters that returns `this`.
jpountz
force-pushed
the
fix/querybuilder_generics
branch
from
35ab316
to
7d87087
Compare
…lastic#18368 Similar reasoning as elastic#18133 but for the aggs API. One important change is that I moved the base PipelineAggregatorBuilder class to the o.e.s.aggregations package instead of o.e.s.aggregations.pipeline so that the create method does not need to be public.
QueryBuilder has generics, but those are never used: all call sites use
QueryBuilder<?>. OnlyAbstractQueryBuilderneeds generics so that the baseclass can contain a default implementation for setters that returns
this.