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

allow to customize the key on a range aggregation #728

Merged
merged 1 commit into from Nov 24, 2014

Conversation

agallou
Copy link
Contributor

@agallou agallou commented Nov 23, 2014

The range aggregation allow to customize the key for each range :

The documentation about this could be found here :
http://www.elasticsearch.org/guide/en/elasticsearch/reference/master/search-aggregations-bucket-range-aggregation.html#_keyed_responsea

Now, the Range aggregation class has a thrird (optionnal) parameter to the addRange method
to passe the range customized key name.

The range aggregation allow to customize the key for each range :

The documentation about this could be found here :
http://www.elasticsearch.org/guide/en/elasticsearch/reference/master/search-aggregations-bucket-range-aggregation.html#_keyed_responsea

Now, the Range aggregation class has a thrird (optionnal) parameter to the `addRange` method
to passe the range customized key name.
@ruflin
Copy link
Owner

ruflin commented Nov 23, 2014

I'm thinking if we should replace all three parameters through a $range array. Like this it is easy to add future params without having to change the code. We can keep BC compatibility for 2-3 version by checking if the first param is an array or not. If not, we can fetch a second param. This would make it more extensible. What do you think?

@agallou
Copy link
Contributor Author

agallou commented Nov 23, 2014

I don't realy have a clear opinion on this :

  • Passing an array is more extensible, but we lost some clarity in the API. It needs documentation instead of simply read the parameters (also in that case the method will just do a $this->addParam('ranges', $range); )
  • Adding another parameter will not be very extensible (if another parameter is added in elasticsearch do we need to add a forth parameter to method ?)

I may tend to think that we could add a third parameter, but I a forth came up, we could reconsider using an array.

So, let me know what to do and i'll will update the PR accoding to your opinion.

ruflin added a commit that referenced this pull request Nov 24, 2014
allow to customize the key on a range aggregation
@ruflin ruflin merged commit f8f1efc into ruflin:master Nov 24, 2014
@ruflin
Copy link
Owner

ruflin commented Nov 24, 2014

I decided to go with the third parameter even though my experience with elasticsearch tells me that there is probably a fourth parameter coming. But then we still have to opportunity to make the change. Thanks for the contribution.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants