-
Notifications
You must be signed in to change notification settings - Fork 25k
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
Use parseField
to parse all parameter names
#8964
Comments
Uses references to XContentParser as a starting point I have created a list of the classes that need to be updated as part of this ticket. There may be some false positives in there (classes which don't need to be updated) but it's hopefully caught everything. https://gist.github.com/colings86/923e3e0a0e87bc4fe10c |
This seems like it'd be covered by or a starting point for the issue for
|
@colings86 Updated your list of classes to touch here: https://gist.github.com/MaineC/2c630dea38077a2852cc (clumsy bash command that generated the list on top of the gist, manually deleted references of aggregations and queries after running the command assuming #14249 and your aggregations refactorings fix those) |
This switches query parsing from manual field parsing to using ParseField. Also adds unit tests for each query that check original json can be parsed into query builders. Relates to elastic#8964
I think this has been resolved with the query refactor? At least when I run @MaineC's script I no longer see any classes that need to be touched. |
Not all queries were migrated to parse field while working on the query refactoring, only some were. Also, using parse field is necessary everywhere in our parsing code, not only in our queries. We should update the list and re-evaluate what needs to be done, but this can't be closed just yet ;) |
I didn't see any more when using Isabel's script, maybe we need to update the script if there are other its missing? |
Looked at the bash thingy again - first of all it only checked stuff in core/*, second of all looking closer it looks like there's some copy'n'paste mix-up in there. Going through the code there still seem to be a few classes where we could be using ParseField, see below. https://gist.github.com/MaineC/ce2bf46d228230a1f676037a40614112 |
Thanks for coming up with the new list @MaineC ! |
That seems to mean that all queries have been migrated though, which I wasn't expecting. that's great! |
Much of the code for parsing parameter names looks like this:
The
parseField
method already handles the parsing of alternatives (including camelCase and deprecated syntax). It also provides a central point for warning when deprecated parameter names are used (see #11033).All existing code should be migrated to use
parseField
.The text was updated successfully, but these errors were encountered: