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

Separate parsing implementation from setter in SearchParseElement #6758

Closed
wants to merge 1 commit into from

Conversation

Projects
None yet
3 participants
@synhershko
Copy link
Contributor

synhershko commented Jul 6, 2014

This, in order to allow reuse of parsing logic by plugins or other internal parts.

Closes #3602

@s1monw

This comment has been minimized.

Copy link
Contributor

s1monw commented Jul 7, 2014

I took a quick look at this though and I think we should keep it simpler and just add a method

public  SearchContextHighlight parse(XContentParser parser, IndexQueryParserService queryParserService) {
// do your q parsing
}

and do this in implementations where is makes sense... I don't think we should do different exception handling at all just stick to what is in there for now and can you try to not make things static here. I also see that they are all private so I wonder how you access them?

@synhershko

This comment has been minimized.

Copy link
Contributor Author

synhershko commented Jul 7, 2014

The reason why I did the re-throwing with another exception is to still be able to throw SearchParseException with the context. I think it's valuable in the exception. If you think I can drop it, that would definitely be easier.

The static methods are factory methods, they should indeed be public, my bad.

@s1monw

This comment has been minimized.

Copy link
Contributor

s1monw commented Jul 7, 2014

fail enough... then lets go with ElasticsearchIllegalArgumentException

@synhershko

This comment has been minimized.

Copy link
Contributor Author

synhershko commented Jul 7, 2014

Done, I'll go ahead and apply this to all the things

@synhershko

This comment has been minimized.

Copy link
Contributor Author

synhershko commented Jul 7, 2014

Applied this in a few more places, however most implementations of SearchParseElement are either too simple for this change (a la VersionParseElement) or are too complex and will require serious refactoring (see SortParseElement for example).

I assume the question is do we want to go farther than where we are now in this PR?

@s1monw

View changes

src/main/java/org/elasticsearch/search/fetch/source/FetchSourceParseElement.java Outdated
context.fetchSourceContext(parseImpl(parser));
}

public static FetchSourceContext parseImpl(XContentParser parser) throws IOException {

This comment has been minimized.

Copy link
@s1monw

s1monw Jul 9, 2014

Contributor

can we name all those public void parse(XContentParser parser) and make them not static?

This comment has been minimized.

Copy link
@synhershko

synhershko Jul 9, 2014

Author Contributor

the idea was to make those usable also without needing to instantiate an instance - what's wrong with statics?

This comment has been minimized.

Copy link
@s1monw

s1monw Jul 15, 2014

Contributor

you can not override them when you subclass which is a pita

This comment has been minimized.

Copy link
@synhershko

synhershko Jul 15, 2014

Author Contributor

fair point. As there are no injected ctors in this PR it does make sense to remove the statics, I'll update the PR

@s1monw

View changes

src/main/java/org/elasticsearch/search/highlight/HighlighterParseElement.java Outdated
}
}

public static SearchContextHighlight parseImpl(XContentParser parser, IndexQueryParserService queryParserService) throws IOException {

This comment has been minimized.

Copy link
@s1monw

s1monw Jul 9, 2014

Contributor

remove the static and name it parse

@s1monw

View changes

src/main/java/org/elasticsearch/search/highlight/HighlighterParseElement.java Outdated
}

private SearchContextHighlight.FieldOptions.Builder parseFields(XContentParser parser, SearchContext context) throws IOException {
private static SearchContextHighlight.FieldOptions.Builder parseFields(XContentParser parser, IndexQueryParserService queryParserService) throws IOException {

This comment has been minimized.

Copy link
@s1monw

s1monw Jul 9, 2014

Contributor

no need to make it static?

@s1monw

View changes

src/main/java/org/elasticsearch/search/rescore/RescoreParseElement.java Outdated
@@ -41,7 +41,7 @@ public void parse(XContentParser parser, SearchContext context) throws Exception
}
}

private void parseSingleRescoreContext(XContentParser parser, SearchContext context) throws Exception {
public static void parseSingleRescoreContext(XContentParser parser, SearchContext context) throws Exception {

This comment has been minimized.

Copy link
@s1monw

s1monw Jul 9, 2014

Contributor

no need for static?

@s1monw

View changes

src/main/java/org/elasticsearch/search/rescore/RescoreParseElement.java Outdated
@@ -70,7 +70,7 @@ private void parseSingleRescoreContext(XContentParser parser, SearchContext cont
throw new ElasticsearchIllegalArgumentException("missing rescore type");
}
if (windowSize != null) {
rescoreContext.setWindowSize(windowSize.intValue());

This comment has been minimized.

Copy link
@s1monw

s1monw Jul 9, 2014

Contributor

I prefer to autobox here

@s1monw

This comment has been minimized.

Copy link
Contributor

s1monw commented Jul 9, 2014

I left some comments... if this is enough for you we can just pull it without changing all the others

@s1monw

This comment has been minimized.

Copy link
Contributor

s1monw commented Jul 15, 2014

@synhershko any news on this?

@s1monw s1monw removed the review label Jul 15, 2014

@synhershko

This comment has been minimized.

Copy link
Contributor Author

synhershko commented Jul 15, 2014

@synhershko

This comment has been minimized.

Copy link
Contributor Author

synhershko commented Jul 16, 2014

@s1monw pushed fixes as per your comments

@s1monw

This comment has been minimized.

Copy link
Contributor

s1monw commented Jul 17, 2014

LGTM can you squash the commits

Separate parsing impl from setter in SearchParseElement
This, in order to allow reuse of parsing logic by plugins or other internal parts
@synhershko

This comment has been minimized.

Copy link
Contributor Author

synhershko commented Jul 18, 2014

@s1monw done

@synhershko

This comment has been minimized.

Copy link
Contributor Author

synhershko commented Jul 27, 2014

@s1monw ping, before this gets stale :)

@s1monw

This comment has been minimized.

Copy link
Contributor

s1monw commented Jul 28, 2014

thanks for the ping I will take care of it soon

@s1monw

This comment has been minimized.

Copy link
Contributor

s1monw commented Jul 28, 2014

I adjusted the commit msg and pushed to master & 1.x thanks @synhershko

@s1monw s1monw closed this Jul 28, 2014

@clintongormley clintongormley changed the title Separate parsing implementation from setter in SearchParseElement Internal: Separate parsing implementation from setter in SearchParseElement Sep 10, 2014

@clintongormley clintongormley changed the title Internal: Separate parsing implementation from setter in SearchParseElement Separate parsing implementation from setter in SearchParseElement Jun 7, 2015

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.