Skip to content
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

Adding fromXContent to Suggestion.Entry and subclasses #23202

Merged
merged 2 commits into from
Feb 16, 2017

Conversation

cbuescher
Copy link
Member

This adds parsing from xContent to Suggestion.Entry and its subclasses for Terms-, Phrase-
and CompletionSuggestion.Entry.

@cbuescher cbuescher added :Search/Search Search-related issues that do not fall into other categories >enhancement review v6.0.0-alpha1 labels Feb 16, 2017
Copy link
Member

@tlrx tlrx left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I left some comments but this is only cosmetics.

}
protected static final String TEXT = "text";
protected static final String OFFSET = "offset";
protected static final String LENGTH = "length";
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think some of them could be private

@@ -551,6 +553,24 @@ public XContentBuilder toXContent(XContentBuilder builder, Params params) throws
return builder;
}

private static ObjectParser<Entry<Option>, Void> PARSER = new ObjectParser<>("SuggestionEntryParser",
true, Entry::new);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you remove the line break?

@@ -100,6 +103,18 @@ public void addOption(Suggestion.Entry.Option option) {
}
}

private static ObjectParser<Entry, Void> PARSER = new ObjectParser<>("PhraseSuggestionEntryParser", true,
Entry::new);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It should fit on 1 line

@@ -154,6 +155,18 @@ protected Option newOption() {
return new Option();
}

private static ObjectParser<Entry, Void> PARSER = new ObjectParser<>("TermSuggestionEntryParser", true,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same


@SuppressWarnings("rawtypes")
private static final Map<Class<? extends Entry>, Function<XContentParser, ? extends Entry>> ENTRY_PARSERS = new HashMap<>();
{
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you make this block static?

/**
* Create a randomized Suggestion.Entry, determined by the type argument.
* @param type determines the entry type for this test. Valid arguments are 0 for {@link TermSuggestion.Entry},
* 1 for {@link PhraseSuggestion.Entry} and 2 for {@link CompletionSuggestion.Entry}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this is out of date?

entry = new Suggestion.Entry<>(entryText, offset, length);
supplier = SuggestionOptionTests::createTestItem;
} else if (entryType == TermSuggestion.Entry.class) {
entry = new TermSuggestion.Entry(entryText, offset, length);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wrong formating

@SuppressWarnings("unchecked")
public void testFromXContent() throws IOException {
for (Class<? extends Entry> entryType : ENTRY_PARSERS.keySet()) {
Entry<Option> entry = createTestItem(entryType);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wrong formating, or maybe you didn't want to iterate over each entry type and randomize it instead?

@cbuescher
Copy link
Member Author

@tlrx thanks for the review, I addressed all of your comments, re-running also locally because CI seems stuck currently.

@cbuescher cbuescher merged commit 268d15e into elastic:master Feb 16, 2017
cbuescher added a commit that referenced this pull request Feb 16, 2017
This adds parsing from xContent to Suggestion.Entry and its subclasses for Terms-, Phrase-
and CompletionSuggestion.Entry.
@cbuescher
Copy link
Member Author

Merged to 5.x with 7c229ce

cbuescher added a commit that referenced this pull request Feb 20, 2017
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.
cbuescher added a commit that referenced this pull request Feb 21, 2017
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.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
>enhancement :Search/Search Search-related issues that do not fall into other categories v5.4.0 v6.0.0-alpha1
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants