-
Notifications
You must be signed in to change notification settings - Fork 24.7k
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
Rename Mapper#name to Mapper#fullPath #110040
Rename Mapper#name to Mapper#fullPath #110040
Conversation
This addresses a long standing TODO that caused quite a few bugs over time, in that the mapper name does not include its full path, while the MappedFieldType name does. We have renamed Mapper.Builder#name to leafName (elastic#109971) and Mapper#simpleName to leafName (elastic#110030). This commit renames Mapper#name to fullPath for clarity This required some adjustments in FieldAliasMapper to avoid confusion between the existing path method and fullPath. I renamed path to targetPath for clarity. ObjectMapper already had a fullPath method that returned name, and was effectively a copy of name, so it could be removed.
Pinging @elastic/es-search (Team:Search) |
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.
LGTM! Thanks Luca for these PRs, the code is much more clear now!
/** | ||
* Returns the name of the field. | ||
* When the field has a parent object, its leaf name won't include the entire path. | ||
* When subobjects are disabled, its leaf name will be the same as {@link #fullPath()} in practice, because its parent is the root. | ||
*/ |
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.
++ on the docs
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.
glad you noticed that in the diff!
This addresses a long standing TODO that caused quite a few bugs over time, in that the mapper name does not include its full path, while the MappedFieldType name does.
We have renamed Mapper.Builder#name to leafName (#109971) and Mapper#simpleName to leafName (#110030). This commit renames Mapper#name to fullPath for clarity This required some adjustments in FieldAliasMapper to avoid confusion between the existing path method and fullPath. I renamed path to targetPath for clarity. ObjectMapper already had a fullPath method that returned name, and was effectively a copy of name, so it could be removed.
Note that this will require adjustments for plugin developers that implement custom mappers. They will need to switch from using
name()
tofullPath()
in their code.