Separate parsing implementation from setter in SearchParseElement #6758
Conversation
|
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 |
|
The reason why I did the re-throwing with another exception is to still be able to throw The static methods are factory methods, they should indeed be public, my bad. |
|
fail enough... then lets go with ElasticsearchIllegalArgumentException |
|
Done, I'll go ahead and apply this to all the things |
|
Applied this in a few more places, however most implementations of I assume the question is do we want to go farther than where we are now in this PR? |
| context.fetchSourceContext(parseImpl(parser)); | ||
| } | ||
|
|
||
| public static FetchSourceContext parseImpl(XContentParser parser) throws IOException { |
There was a problem hiding this comment.
can we name all those public void parse(XContentParser parser) and make them not static?
There was a problem hiding this comment.
the idea was to make those usable also without needing to instantiate an instance - what's wrong with statics?
There was a problem hiding this comment.
you can not override them when you subclass which is a pita
There was a problem hiding this comment.
fair point. As there are no injected ctors in this PR it does make sense to remove the statics, I'll update the PR
|
I left some comments... if this is enough for you we can just pull it without changing all the others |
|
@synhershko any news on this? |
|
@s1monw pushed fixes as per your comments |
|
LGTM can you squash the commits |
This, in order to allow reuse of parsing logic by plugins or other internal parts
|
@s1monw done |
|
@s1monw ping, before this gets stale :) |
|
thanks for the ping I will take care of it soon |
|
I adjusted the commit msg and pushed to master & 1.x thanks @synhershko |
clintongormley
changed the title
clintongormley
changed the title
This, in order to allow reuse of parsing logic by plugins or other internal parts.
Closes #3602