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

Replace parameter unicodeSetFilter with unicode_set_filter #29215

Merged
merged 4 commits into from
Nov 1, 2018

Conversation

liketic
Copy link

@liketic liketic commented Mar 23, 2018

Depreate parameter unicodeSetFilter and replace it with unicode_set_filter for IcuFoldingTokenFilter.

Close #22823

@elasticmachine
Copy link
Collaborator

Since this is a community submitted pull request, a Jenkins build has not been kicked off automatically. Can an Elastic organization member please verify the contents of this patch and then kick off a build manually?

1 similar comment
@elasticmachine
Copy link
Collaborator

Since this is a community submitted pull request, a Jenkins build has not been kicked off automatically. Can an Elastic organization member please verify the contents of this patch and then kick off a build manually?

@nik9000 nik9000 added the :Search Relevance/Analysis How text is split into tokens label Mar 23, 2018
@elasticmachine
Copy link
Collaborator

Pinging @elastic/es-search-aggs

@javanna
Copy link
Member

javanna commented May 7, 2018

@elastic/es-search-aggs thoughts on this?

@javanna javanna added the review label May 7, 2018
@rjernst rjernst removed the review label Oct 10, 2018
@colings86
Copy link
Contributor

@romseygeek would you be able to pick up reviewing this PR?

@romseygeek
Copy link
Contributor

I think this looks good, @liketic would you be able to update this to the latest master and push again?

@liketic
Copy link
Author

liketic commented Oct 27, 2018

Thanks @romseygeek , I updated the branch, please review again.

@romseygeek
Copy link
Contributor

@elasticmachine test this please

@romseygeek
Copy link
Contributor

Looks good, thanks @liketic. One more thing, could you update docs/plugins/analysis-icu.asciidoc to replace all instances of unicodeSetFilter with unicode_set_filter?

@romseygeek romseygeek self-assigned this Oct 29, 2018
@romseygeek romseygeek merged commit 14f540e into elastic:master Nov 1, 2018
@romseygeek
Copy link
Contributor

Thanks @liketic !

@@ -35,14 +38,15 @@
* <p>The {@code unicodeSetFilter} attribute can be used to provide the UniCodeSet for filtering.</p>
*/
public class IcuNormalizerTokenFilterFactory extends AbstractTokenFilterFactory implements MultiTermAwareComponent {

private final static DeprecationLogger deprecationLogger =
Copy link
Contributor

Choose a reason for hiding this comment

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

@romseygeek this line is causing checkstyle violations

> Task :plugins:analysis-icu:checkstyleMain FAILED
[ant:checkstyle] [ERROR] /home/alpar/work/elastic/elasticsearch/plugins/analysis-icu/src/main/java/org/elasticsearch/index/analysis/IcuNormalizerTokenFilterFactory.java:41:19: 'static' modifier out of order with the JLS suggestions. [ModifierOrder]

I'm pushing a fix to master now. FYI

Copy link
Contributor

Choose a reason for hiding this comment

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

Master was merged in a few days ago and the checkstyle rules last changed 4 weeks ago, yet the PR build passed.

Copy link
Contributor

Choose a reason for hiding this comment

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

I opened #35207 to fix these

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.

9 participants