Drop name from TokenizerFactory#24869
Merged
nik9000 merged 1 commit intoelastic:masterfrom May 30, 2017
Merged
Conversation
Drops `TokenizerFactory#name`, replacing it with `CustomAnalyzer#getTokenizerName` which is much better targeted at its single use case inside the analysis API. Drops a test that I would have had to refactor which is duplicated by `AnalysisModuleTests`. To keep this change from blowing up in size I've left two mostly mechanical changes to be done in followups: 1. `TokenizerFactory` can now be entirely dropped and replaced with `Supplier<Tokenizer>`. 2. `AbstractTokenizerFactory`'s ctor still takes a `String` parameter where the name once was.
rjernst
approved these changes
May 25, 2017
Member
rjernst
left a comment
There was a problem hiding this comment.
LGTM, I just left one comment.
| /** | ||
| * The name of the tokenizer as configured by the user. | ||
| */ | ||
| public String getTokenizerName() { |
Member
There was a problem hiding this comment.
Why do we need this? The TokenizerFactory.name() was never used here before.
Member
Author
There was a problem hiding this comment.
It was used in the analyze action.
Member
Author
|
Thanks for the review @rjernst! I merged and added the followups to my list. |
jasontedor
added a commit
to jasontedor/elasticsearch
that referenced
this pull request
May 30, 2017
* master: Fix typo in comment in ReplicationOperation.java Prevent Index & Delete request primaryTerm getter/setter, setShardId setter Drop name from TokenizerFactory (elastic#24869) Correctly set doc_count when MovAvg "predicts" values on existing buckets (elastic#24892) Handle primary failure handling replica response Add missing word to terms-query.asciidoc (elastic#24960) Correct some spelling in match-phrase-prefix docs (elastic#24956) testConcurrentWriteViewsAndSnapshot shouldn't flush concurrently [TEST] Fix FieldSortIT failures Add doc_count to ParsedMatrixStats (elastic#24952) Add document count to Matrix Stats aggregation response (elastic#24776) Fix script field sort returning Double.MAX_VALUE for all documents (elastic#24942)
nik9000
added a commit
to nik9000/elasticsearch
that referenced
this pull request
May 30, 2017
A continuation of elastic#24869, this drops the `name()` method from all remaining analysis component factories, moving name members to `CustomAnalayzer` in service of the `_analyze` action. Like elastic#24869 I've kept a `String` in the method signature of the ctor of some abstract components so that back these analysis component factories. It is *much* more convenient to remove all the names at once in a followup. That change will be large but purely mechanical. This breaks the API for plugins adding analyzers because they no longer must implement fairly redundant feeline method.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Drops
TokenizerFactory#name, replacing it withCustomAnalyzer#getTokenizerNamewhich is much better targeted atits single use case inside the analysis API.
Drops a test that I would have had to refactor which is duplicated by
AnalysisModuleTests.To keep this change from blowing up in size I've left two mostly
mechanical changes to be done in followups:
TokenizerFactorycan now be entirely dropped and replaced withSupplier<Tokenizer>.AbstractTokenizerFactory's ctor still takes aStringparameterwhere the name once was.