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

Encapsulate AggregationBuilder name and make getter public #7425

Closed
wants to merge 1 commit into from

Conversation

Projects
None yet
3 participants
@philwills
Copy link
Contributor

philwills commented Aug 24, 2014

As mentioned in https://groups.google.com/forum/?#!msg/elasticsearch/fe9S2mECMEI/57xZK7i6V7MJ, I'd like the name of an AggregationBuilder to be public. I'm attempting to build a wrapper that will provide some type safety around the type of aggregation in the query and the result and not having access to the name is making the API much clunkier than it would otherwise be.

@jpountz

This comment has been minimized.

Copy link
Contributor

jpountz commented Aug 26, 2014

Hi @philwills, thanks for opening this PR. Maybe there should be a getter (getName) for this property instead of making it public? It would also be nice to make sure this method is used at least somewhere, so maybe we should do the following:

  • make name private in AggregationBuilder
  • add a getter for name (getName()) in AggregationBuilder
  • fix remaining compilation errors by replacing occurrences of name from sub-classes with getName?

Additionally, could you please sign our contributor license agreement?

Thanks!

@jpountz jpountz removed the review label Aug 26, 2014

@philwills

This comment has been minimized.

Copy link
Contributor Author

philwills commented Aug 26, 2014

Thanks for taking a look @jpountz

Personally, I find getters a bit redundant on a final field, but I'm more than happy to go with the house style. I'll try and put something together this evening.

I've now signed the CLA.

@philwills philwills force-pushed the philwills:public-aggregation-name branch to 719d75e Aug 26, 2014

@philwills

This comment has been minimized.

Copy link
Contributor Author

philwills commented Aug 26, 2014

Now updated as per your suggestion.

@jpountz

This comment has been minimized.

Copy link
Contributor

jpountz commented Aug 28, 2014

@philwills Merged, thanks!

@clintongormley clintongormley changed the title Make the name of an aggregation builder public Aggregations: Encapsulate AggregationBuilder name and make getter public Sep 8, 2014

@clintongormley clintongormley changed the title Aggregations: Encapsulate AggregationBuilder name and make getter public Encapsulate AggregationBuilder name and make getter public 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.