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

CustomAnalyzer ctor to set AnalyzerBase.Type #431

Merged
merged 2 commits into from Jan 16, 2014

Conversation

Projects
None yet
2 participants
@sschlesier
Copy link
Contributor

commented Dec 31, 2013

It is not obvious that the Type property should be set on a class that inherits from CustomAnalyzer.

By providing a ctor that sets the Type property users will not need to remember
to set the type property manually.

Removing the default ctor is ideal, but would be a breaking change. Marking it obsolete
should lead most people to "the pit of success".

CustomAnalyzer ctor to set AnalyzerBase.Type
By providing a ctor that sets the Type property users will not need to remember
to set the type property manually.

Removing the default ctor would be a breaking change.  Marking it obsolete
should lead most people to "the pit of success".
@Mpdreamz

This comment has been minimized.

Copy link
Member

commented Jan 2, 2014

Hi @sschlesier

Subclassing CustomAnalyzer is not common i,e, all my custom analyzers use CustomAnalyzer directly:

.Add("folded_keyword", new CustomAnalyzer()
{
    Tokenizer = "keyword",
    Filter = new List<string> { "lowercase", "asciifolding" }
})

With that in mind obsoleting the default constructor does not seem preferable. What's your use case that you find the need to inherit CustomAnalyzer?

@Mpdreamz

This comment has been minimized.

Copy link
Member

commented Jan 13, 2014

Hi @sschlesier

any update on this would be appreciated!

@sschlesier

This comment has been minimized.

Copy link
Contributor Author

commented Jan 14, 2014

Out existing code base does the majority of the configuration with the fluent API, but the CustomAnalyzers are written as classes derived from CustomAnalyzer. Dealing the the Type string is awkward when deriving from CustomAnalyzer, hence the PR.

If you don't think deriving from CustomAnalyzer is a good idea, feel free to close this PR.

@Mpdreamz

This comment has been minimized.

Copy link
Member

commented Jan 14, 2014

@sschlesier I don't mind pulling it in with the added constructor but without obsoleting the default. Would that work for you?

@sschlesier

This comment has been minimized.

Copy link
Contributor Author

commented Jan 15, 2014

@Mpdreamz I have removed the obsolete attribute, and will be pleased if you accept this PR.

Mpdreamz added a commit that referenced this pull request Jan 16, 2014

Merge pull request #431 from sschlesier/CustomAnalyzer-ctor
CustomAnalyzer ctor to set AnalyzerBase.Type

@Mpdreamz Mpdreamz merged commit 254d673 into elastic:master Jan 16, 2014

@Mpdreamz

This comment has been minimized.

Copy link
Member

commented Jan 16, 2014

Done, keep'm coming 👍 and I always love to hear about how people are using NEST so thanks for that.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.