Skip to content

Implements creating a term suggester from x-content#16483

Merged
abeyad merged 1 commit intoelastic:feature-suggest-refactoringfrom
abeyad:feature-term-suggester-xcontent
Feb 9, 2016
Merged

Implements creating a term suggester from x-content#16483
abeyad merged 1 commit intoelastic:feature-suggest-refactoringfrom
abeyad:feature-term-suggester-xcontent

Conversation

@abeyad
Copy link
Copy Markdown

@abeyad abeyad commented Feb 5, 2016

No description provided.

@abeyad abeyad force-pushed the feature-term-suggester-xcontent branch 2 times, most recently from 8ff1c12 to 57f1d29 Compare February 8, 2016 15:01
@cbuescher cbuescher added review :Search Relevance/Suggesters "Did you mean" and suggestions as you type labels Feb 9, 2016
@cbuescher
Copy link
Copy Markdown
Member

@abeyad I took a first look and only left a few minor things as comments. Can you also activate the testFromXContent() tests in TermSuggestionBuilderTests? When I did that they were still failing for me. Maybe you can take a look, I guess there might be still some things missing from the parser or some parameters not surviving the roundtrip correctly.

@abeyad abeyad force-pushed the feature-term-suggester-xcontent branch from 57f1d29 to ab43cb8 Compare February 9, 2016 19:52
@abeyad
Copy link
Copy Markdown
Author

abeyad commented Feb 9, 2016

@cbuescher I implemented the changes and all tests pass now.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

nit: we almost never use Locale.US exept for some number formating. While I think it doesn't make a difference for the enum names in question here, I'd suggest going with Locale.ROOT

@abeyad abeyad force-pushed the feature-term-suggester-xcontent branch from ab43cb8 to 8c00f36 Compare February 9, 2016 20:09
@abeyad
Copy link
Copy Markdown
Author

abeyad commented Feb 9, 2016

@cbuescher I changed all Locale.US to Locale.ROOT

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Why these changes? I first thought it's because the method rename but it seems unrelated to your change now. If so, can you change this back?

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

good catch, it was my IDE, my fault

@abeyad abeyad force-pushed the feature-term-suggester-xcontent branch from 8c00f36 to 7ca7254 Compare February 9, 2016 20:38
@abeyad
Copy link
Copy Markdown
Author

abeyad commented Feb 9, 2016

@cbuescher made the changes

@cbuescher
Copy link
Copy Markdown
Member

Thanks, LGTM now.

abeyad pushed a commit that referenced this pull request Feb 9, 2016
@abeyad abeyad merged commit e82713a into elastic:feature-suggest-refactoring Feb 9, 2016
@abeyad abeyad changed the title Feature term suggester xcontent Implements creating a term suggester from x-content Feb 9, 2016
@clintongormley clintongormley added :Search/Search Search-related issues that do not fall into other categories and removed :Search Refactoring labels Feb 14, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

:Search Relevance/Suggesters "Did you mean" and suggestions as you type :Search/Search Search-related issues that do not fall into other categories

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants