Skip to content

Move more token filters to analysis-common module#25214

Merged
martijnvg merged 1 commit intoelastic:masterfrom
martijnvg:move_more_token_filters_to_analyis-commons_module
Jun 15, 2017
Merged

Move more token filters to analysis-common module#25214
martijnvg merged 1 commit intoelastic:masterfrom
martijnvg:move_more_token_filters_to_analyis-commons_module

Conversation

@martijnvg
Copy link
Copy Markdown
Member

The following token filters were moved: edge_ngram, ngram, uppercase, lowercase, length, flatten_graph and unique.

Relates to #23658

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Added a general ngram search rest test to analysis module, so the removal of these lines and above is covered.

Copy link
Copy Markdown
Member Author

@martijnvg martijnvg Jun 14, 2017

Choose a reason for hiding this comment

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

The useAllFields(...) is now tested by changing the default search field to f1 instead of using a custom analyzer with ngram token filters.

Copy link
Copy Markdown
Contributor

@jpountz jpountz left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

++

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Not a blocker but I think it would be best as a rest-test. My reasoning is that if analysis-common starts depending on highlighters, then highlighters could not be moved to a module, which is something we might want to do at some point.

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.

I think it'd be nice if this were a rest test too. It can be a java rest test instead of a yaml rest test if that is easier.

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.

👍

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.

I believe @andy-elastic started a trend to remove the extra spaces. Can you remove the extra spaces in this map while you are modifying it?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

👍

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.

I think it'd be nice if this were a rest test too. It can be a java rest test instead of a yaml rest test if that is easier.

@martijnvg martijnvg force-pushed the move_more_token_filters_to_analyis-commons_module branch from b24bea6 to 892c606 Compare June 15, 2017 14:36
The following token filters were moved: `edge_ngram`, `ngram`, `uppercase`, `lowercase`, `length`, `flatten_graph` and `unique`.

Relates to elastic#23658
@martijnvg martijnvg force-pushed the move_more_token_filters_to_analyis-commons_module branch from 892c606 to 428e707 Compare June 15, 2017 16:28
@martijnvg martijnvg merged commit 428e707 into elastic:master Jun 15, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants