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

Avoid deprecation with interval for DateHistogram #2099

Closed

Conversation

VincentLanglet
Copy link
Contributor

This could be a way to solve #2097

src/Aggregation/DateHistogram.php Outdated Show resolved Hide resolved
src/Aggregation/DateHistogram.php Outdated Show resolved Hide resolved
@franmomu
Copy link
Contributor

franmomu commented Jul 12, 2022

what about creating setCalendarInterval, setFixedInterval and deprecate setInterval? to be in sync with elasticsearch

@VincentLanglet
Copy link
Contributor Author

what about creating setCalendarInterval, setFixedInterval and deprecate setInterval? to be in sync with elasticsearch

DateHistogram extends Histogram, and the Histogram constructor expects a interval value as third parameter (and then call setInterval in the constructor).
I thought about adding a 4th parameter to set if it's calendar or fixed, but this can automatically be determined like I did, so it's kinda useless.
Also, the setInterval method is coming from the Histogram class. So if you want to remove it, you need to stop extending Histogram.

An other strategy started in #1874 but never ended...

@thePanz
Copy link
Collaborator

thePanz commented Jul 16, 2022

I like this approach, later we could deprecate the setInterval.

We should also add the setCalendarInterval and setFixedInterval methods as suggested by @franmomu, to allow users to properly set the intervals following the ES documentation.

WDYT?

Later we could even have 2 different classes implementing the CalendarIntervaDateHistograml and FixedIntervalDateHistogram, so that we could enforce the creation of valid objects.
This would be a topic for another PR, imho.

@VincentLanglet
Copy link
Contributor Author

I like this approach, later we could deprecate the setInterval.

We should also add the setCalendarInterval and setFixedInterval methods as suggested by @franmomu, to allow users to properly set the intervals following the ES documentation.

WDYT?

Done @thePanz

src/Aggregation/DateHistogram.php Outdated Show resolved Hide resolved
src/Aggregation/DateHistogram.php Outdated Show resolved Hide resolved
tests/Aggregation/DateHistogramTest.php Show resolved Hide resolved
@VincentLanglet VincentLanglet requested a review from thePanz July 19, 2022 15:58
tests/Aggregation/DateHistogramTest.php Outdated Show resolved Hide resolved
tests/Aggregation/DateHistogramTest.php Outdated Show resolved Hide resolved
@VincentLanglet VincentLanglet requested a review from thePanz July 20, 2022 08:55
@VincentLanglet VincentLanglet requested a review from thePanz July 20, 2022 09:15
@thePanz
Copy link
Collaborator

thePanz commented Jul 22, 2022

LGTM, thanks @VincentLanglet!
Would you add the changes to the Changelog?

@thePanz
Copy link
Collaborator

thePanz commented Jul 23, 2022

Merged in d4849e2
Thanks @VincentLanglet !

@thePanz thePanz closed this Jul 23, 2022
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.

4 participants