Simplify generics on Mapper.Builder #56747
Conversation
romseygeek
added
:Search Foundations/Mapping
|
Pinging @elastic/es-search (:Search/Mapping) |
|
I think we can probably remove the first generic parameter as well, if we're willing to give up on fluent builders - it adds a lot of complexity and visual noise for not much return, IMO. |
There was a problem hiding this comment.
Great simplification, I was looking for the spots where we actually needed that second parameters and could only find one where I'm even not sure (left a short question just for my understanding). LGTM from my side.
I have one question regarding backport to 7.9 though: are we allowing users to define and add custom field types to Elasticsearch, e.g. via Plugins? In this case this might be a breaking change I think. Not sure about this though.
| public Builder(String name, MappedFieldType fieldType, MappedFieldType defaultFieldType) { | ||
| super(name, fieldType, defaultFieldType); | ||
| } | ||
|
|
||
| public abstract MetadataFieldMapper build(BuilderContext context); |
There was a problem hiding this comment.
just out of curiosity: this seems to be one of the few reasons where the generic build return type was needed?
There was a problem hiding this comment.
There are a few places in tests as well that expect build() to return a specific type, but nothing within the server code itself.
Yes, this would be a breaking change, but I think we allow these in point releases? That's why plugins have to be tied to a specific release. I've actually tried removing the first generic parameter as well, and that adds very little noise to the PR but makes things a whole lot simpler still - should I include that here, or open up a followup do you think? |
Tbh I'm not sure, just wanted to bring this up since it came to mind. Maybe needs another opinion.
I wouldn't mind if its in a separate commit. Depending on the diff it might make sense to open another PR though, also maybe for folks who e.g. later want to track the history of an API change. |
|
It's mostly changes on the same lines as this current one - I'll push the new changes here. |
|
Have reverted the complete generics removal, and I'll open a separate issue for that. |
Mapper.Builder currently has some complex generics on it to allow fluent builder construction. However, the second parameter, a return type from the build() method, is unnecessary, as we can use covariant return types. This commit removes this second generic parameter.
We simplified the generics on Mapper.Builder in #56747, but stopped short of removing them entirely because they were still used in various places in the code. Now that most field mappers have been converted to parametrized form, these generics are no longer useful. There are very few places where a fluent Builder pattern is used, almost all in tests, and these can all be replaced with simple casts; in exchange, we remove lots of visual cruft and clean up a number of warnings.
We simplified the generics on Mapper.Builder in #56747, but stopped short of removing them entirely because they were still used in various places in the code. Now that most field mappers have been converted to parametrized form, these generics are no longer useful. There are very few places where a fluent Builder pattern is used, almost all in tests, and these can all be replaced with simple casts; in exchange, we remove lots of visual cruft and clean up a number of warnings.
Mapper.Builder currently has some complex generics on it to allow fluent builder
construction. However, the second parameter, a return type from the
build()method,is unnecessary, as we can use covariant return types. This commit removes this second
generic parameter.