add Extended Stats Bucket #1756
Conversation
|
|
||
| class ExtendedStatsBucketTest extends BaseAggregationTest | ||
| { | ||
| protected function _getIndexForTest(): Index |
There was a problem hiding this comment.
Please use private as method visibility here
There was a problem hiding this comment.
This is still protected :)
There was a problem hiding this comment.
This is till protected, it should be private instead
|
Hello @thePanz I removed all useless comments and annotations ;) Thx ! |
|
Seems CI is not happy :-( @thePanz Feel free to merge as soon as you are happy and CI is green. |
|
Hey @thePanz Everything is ok for you ? |
| */ | ||
| class ExtendedStatsBucket extends AbstractAggregation | ||
| { | ||
| public function __construct(string $name, ?string $bucketsPath = null) |
There was a problem hiding this comment.
I just noticed it now: why is the bucketPath not required in the constructor, but later required when building the request( toArray())?
As it is required, I'd make it required (not empty) and remove the setBucketPath() method. What do you think?
There was a problem hiding this comment.
@thePanz Perhaps the same argument as in the other PR might apply. No strong opinion which on is better but we should become consistent.
There was a problem hiding this comment.
i followed the model of the StatsBucket aggregation :/
But both of you are right. We can make the $bucketsPath argument mandatory and remove the setter in the 2 classes :)
There was a problem hiding this comment.
@nicolassing : I'd leave the setters (and do the "not empty" check there, throwing an exception)
and make the argument required in the constructor
I'll cleanup the other aggregations @ruflin , but in another PR. I'd like not to have devs creating invalid objects to start with :) but this is a topic for a discussion, not here tho :)
There was a problem hiding this comment.
But first: let's have the $bucketsPath argument as required :)
|
@nicolassing Could you add a changelog entry? I think then this PR is good to go. |
| */ | ||
| public function toArray(): array | ||
| { | ||
| if (!$this->hasParam('buckets_path')) { |
There was a problem hiding this comment.
This is not needed anymore
There was a problem hiding this comment.
Yes ! I was correcting it ^^
| { | ||
| parent::__construct($name); | ||
|
|
||
| if (null !== $bucketsPath) { |
There was a problem hiding this comment.
the if condition is now always true
|
Merged! thanks @nicolassing for the PR, the changes and the patience 👍 |
No description provided.