Drop name from TokenizerFactory #24869
Merged
Conversation
This file contains 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
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.
nik9000
added
:Core/Infra/Plugins
rjernst
approved these changes
May 25, 2017
| /** | ||
| * The name of the tokenizer as configured by the user. | ||
| */ | ||
| public String getTokenizerName() { |
There was a problem hiding this comment.
Why do we need this? The TokenizerFactory.name() was never used here before.
There was a problem hiding this comment.
It was used in the analyze action.
|
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.
original-brownbear
added a commit
to original-brownbear/elasticsearch
that referenced
this pull request
Jul 15, 2018
Handles TODOs from elastic#24869 * Replaces all occurences of TokenizerFactory with Supplier<Tokenizer> * Remove unused parameter from constructor
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.