Skip to content

Refactors SpanTermQueryBuilder.#11005

Merged
MaineC merged 2 commits intoelastic:feature/query-refactoringfrom
MaineC:feature/span-term-query-refactoring
May 18, 2015
Merged

Refactors SpanTermQueryBuilder.#11005
MaineC merged 2 commits intoelastic:feature/query-refactoringfrom
MaineC:feature/span-term-query-refactoring

Conversation

@MaineC
Copy link
Copy Markdown

@MaineC MaineC commented May 6, 2015

Refactors SpanTermQueryBuilder analogous to TermQueryBuilder. Factors common code between the two into separate classes.

Relates to #10217

This goes against the feature/query-refactoring branch.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not sure, but if we use parserName() instead of the constant here, can we move the whole doXContent method do the BaseTermQueryBuilder?

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good point. Will check.

@MaineC MaineC force-pushed the feature/span-term-query-refactoring branch from e20b6b4 to 24d7ae5 Compare May 7, 2015 07:32
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think those two can stay private, or do you need them in the concrete test classes? They should also be accesible via the context you get from createContext() if you need them.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This must have been a merge artifact - will revert, thanks for spotting this.

@cbuescher
Copy link
Copy Markdown
Member

Left some super minor comments, I like the way the similarities in TermQuery and SpanTermQuery are factored out here but still I'd like to hear what @javanna or @dakrone think about this, especially the use of generics.

@MaineC
Copy link
Copy Markdown
Author

MaineC commented May 8, 2015

@cbuescher Thanks for your comments - changed the code accordingly.

@cbuescher
Copy link
Copy Markdown
Member

Thanks, LGTM, but lets see if anybody else has an opinion.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this makes sense, wonder if the comment is even needed, seems more like a commit message ;)

@javanna
Copy link
Copy Markdown
Contributor

javanna commented May 8, 2015

left some comments

@MaineC MaineC force-pushed the feature/span-term-query-refactoring branch from 8908571 to da2eb5a Compare May 11, 2015 09:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

:Search/Search Search-related issues that do not fall into other categories

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants