Skip to content

Conversation

colings86
Copy link
Contributor

No description provided.

return new GeoCentroidAggregator.Factory(aggregationName, vsParser.input());
protected boolean token(String aggregationName, String currentFieldName, Token token, XContentParser parser,
ParseFieldMatcher parseFieldMatcher, Map<ParseField, Object> otherOptions) throws IOException {
return false;
Copy link
Member

Choose a reason for hiding this comment

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

nit: Could you add some javadoc to the abstract method in AbstractValuesSourceParser for the expected behaviour for future reference? It's not so difficult to figure out when reading the parser code but especially with boolean return types, the semantics of true/false tend to get confusing.

Copy link
Contributor

Choose a reason for hiding this comment

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

+1

@cbuescher
Copy link
Member

@colings86 left two small comments, otherwise looks good. Again, not sure if maybe also @jpountz should take a brief look?

@jpountz
Copy link
Contributor

jpountz commented Nov 25, 2015

LGTM, I just seconded @cbuescher 's request to have a bit more of javadocs.

@colings86 colings86 closed this Nov 26, 2015
@colings86
Copy link
Contributor Author

Despite what Github may think, this was successfully merged into the agg refactoring branch 68f18e9

@colings86 colings86 deleted the refactor/geoCentroidAgg branch November 26, 2015 09:52
@clintongormley clintongormley added :Search/Search Search-related issues that do not fall into other categories and removed :Search Refactoring labels Feb 14, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
:Analytics/Aggregations Aggregations :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