Adding fromXContent to Suggest and Suggestion class #23226
Conversation
cbuescher
added
:Search/Search
| suggestion = new CompletionSuggestion(name, -1); | ||
| entryParser = CompletionSuggestion.Entry::fromXContent; | ||
| break; | ||
| } |
There was a problem hiding this comment.
shall we add a TODO that this switch won't be needed once we introduce NamedXContentRegistry?
There was a problem hiding this comment.
Will add a TODO to re-evaluate this. I'm not sure this fits in 100% with NamedXContentRegistry since we not only need to look up a parser but also the right constructor for the suggestion. I will add this to the comment.
| Function<XContentParser, Entry> entryParser = null; | ||
| /// the "size" parameter and the SortBy for TermSuggestion cannot be parsed from the response, use default values | ||
| switch (type) { | ||
| case Suggestion.NAME: |
| } | ||
| if (rarely()) { | ||
| numEntries = 0; // also occasionally test zero entries | ||
| } |
There was a problem hiding this comment.
can you clarify the logic here and assign the variable only once?
|
|
||
| public class SuggestionTests extends ESTestCase { | ||
|
|
||
| @SuppressWarnings({ "unchecked", "rawtypes" }) |
There was a problem hiding this comment.
do you get a warning without the rawtypes suppression?
There was a problem hiding this comment.
No I don't. Ide or command line?
There was a problem hiding this comment.
Scratch that, I misread: yes I get a warning on the map type ( Class<Suggestion>[]) but there seems to be no easy way around this. The whole generic usage in the Suggestion/Entry/Option hierarchy needs some closer look but this would be out of scope for this PR I think.
There was a problem hiding this comment.
I see that in this particular case I can remove the rawtypes suppression if I do Class<Suggestion<? extends Entry<? extends Option>>>[] - not nice but I'll update that.
There was a problem hiding this comment.
I was just wondering if it's IDE configuration. does everything compile and build without the suppression? Asking because I am not sure we generally add those type of suppressions, but I may be wrong.
|
@javanna thanks for the review, I added a small update and will wait for CI to finish |
cbuescher
force-pushed
the
addParsing-suggestions
branch
from
c3bd21d
to
0ed7a2b
Compare
A follow up to #23202, this adds parsing from xContent and tests to the four Suggestion implementations and the top level suggest element to be used later when parsing the entire SearchResponse.
|
Merged to 5.x with 1b44fe5 |
A follow up to #23202, this adds parsing from xContent and tests to the four Suggestion implementations
and the top level
suggestelement to be used later when parsing the entire SearchResponse.