-
Notifications
You must be signed in to change notification settings - Fork 481
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
add function to get the next holiday #2211
base: dev
Are you sure you want to change the base?
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## dev #2211 +/- ##
=========================================
Coverage 100.00% 100.00%
=========================================
Files 196 196
Lines 11849 11865 +16
Branches 1710 1714 +4
=========================================
+ Hits 11849 11865 +16 ☔ View full report in Codecov by Sentry. |
@Rosi2143, it's good start! But some checks are required. Try to run this: from holidays.countries.ukraine import UA
ua = UA()
ua.get_next_holiday("1991-01-01", True)
ua.get_next_holiday("2022-03-08") |
@KJhellico: Thanks - I didn't know that there is a start/end-date for a calender. But I guess for some countries it makes sense. Added the checks and the documentation with the latest version. |
053f942
to
562bfa1
Compare
562bfa1
to
c91e03e
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@Rosi2143, , well done! Please take a look at a few suggestions.
b353dde
to
a5554fb
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's a great speed improvement! 👍
a5554fb
to
c9cd832
Compare
c9cd832
to
adf4653
Compare
Since the holidays in the holidays entity object are not always placed in chronological order, it's necessary to sort them before searching: >>> from holidays.countries.united_states import US
>>> h = US(years=2024)
>>> h.get_next_holiday("2024-02-01")
(datetime.date(2024, 5, 27), 'Memorial Day')
>>> for d, name in h.items():
>>> print(d, name)
2024-01-01 New Year's Day
2024-05-27 Memorial Day
2024-06-19 Juneteenth National Independence Day
2024-07-04 Independence Day
2024-09-02 Labor Day
2024-11-11 Veterans Day
2024-11-28 Thanksgiving
2024-12-25 Christmas Day
2024-01-15 Martin Luther King Jr. Day
2024-02-19 Washington's Birthday
2024-10-14 Columbus Day |
8934029
to
e56effc
Compare
Thanks for the hint that no sorting is applied to calendars. That was one of the reasons I choose my initial implementation. I wanted to keep all the corner cases out of this function - at the cost of having a longer runtime. But fixed this now as well. |
if not previous: | ||
next_date = next((x for x in self.get_entries_sorted() if x > dt), None) | ||
if not next_date and dt.year < self.end_year: | ||
self._populate(dt.year + 1) | ||
next_date = next((x for x in self.get_entries_sorted() if x > dt), None) | ||
else: | ||
next_date = next((x for x in reversed(self.get_entries_sorted()) if x < dt), None) | ||
if not next_date and dt.year > self.start_year: | ||
self._populate(dt.year - 1) | ||
next_date = next((x for x in reversed(self.get_entries_sorted()) if x < dt), None) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if not previous: | |
next_date = next((x for x in self.get_entries_sorted() if x > dt), None) | |
if not next_date and dt.year < self.end_year: | |
self._populate(dt.year + 1) | |
next_date = next((x for x in self.get_entries_sorted() if x > dt), None) | |
else: | |
next_date = next((x for x in reversed(self.get_entries_sorted()) if x < dt), None) | |
if not next_date and dt.year > self.start_year: | |
self._populate(dt.year - 1) | |
next_date = next((x for x in reversed(self.get_entries_sorted()) if x < dt), None) | |
if not previous: | |
next_date = next((x for x in sorted(self.keys()) if x > dt), None) | |
if not next_date: | |
self._populate(dt.year + 1) | |
next_date = next((x for x in sorted(self.keys()) if x > dt), None) | |
else: | |
next_date = next((x for x in sorted(self.keys(), reverse=True) if x < dt), None) | |
if not next_date: | |
self._populate(dt.year - 1) | |
next_date = next((x for x in sorted(self.keys(), reverse=True) if x < dt), None) |
It looks faster and doesn't involve additional functions. And don't squash changes into single commit every time, please.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for working on this @Rosi2143
Please consider these comments:
holidays/holiday_base.py
Outdated
(if previous is False) or the previous holiday (if previous is True). | ||
If no date is given the search starts from current date""" | ||
|
||
dt = self.__keytransform__(start) if start else datetime.now().date() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This may automatically expand years and populate holidays. It seems we want just a conversion here? If not then datetime.now().date()
needs to be treated the same way.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You are right - when the calendar is empty datetime.now()
needs to be tranformed as well.
Thanks.
@@ -959,6 +959,29 @@ def get_named( | |||
|
|||
raise AttributeError(f"Unknown lookup type: {lookup}") | |||
|
|||
def get_next_holiday( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure (as I typically am) about naming here. I read get next
but then I see previous
param.
Let's consider get_closest_holiday
with a param in_future=True
or similar?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think get_closest_holiday
is good - but would rather use parameter in_past=False
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wonder what's the rationale behind this?
@@ -959,6 +959,29 @@ def get_named( | |||
|
|||
raise AttributeError(f"Unknown lookup type: {lookup}") | |||
|
|||
def get_next_holiday( | |||
self, start: DateLike = None, previous: bool = False | |||
) -> Union[tuple[date, str], tuple[None, None]]: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we return just None
instead of tuple[None, None)
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fine with me - but it was you proposal 😄 : #1825 (comment)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, you've implemented it correctly with Union[tuple[date, str]
.
For the no result cases simple None
is enough.
holidays/holiday_base.py
Outdated
self._populate(dt.year + 1) | ||
next_date = next((x for x in sorted(self.keys()) if x > dt), None) | ||
else: | ||
next_date = next((x for x in sorted(self.keys(), reverse=True) if x < dt), None) | ||
if not next_date: | ||
self._populate(dt.year - 1) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It looks like the logic covers next/previous years holidays only. There is a corner case with at least UA
holidays when a year doesn't have any. This would return nothing even after holidays are restored.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
But if we want to implement it that way I'd suggest something similar to this (using bisect module):
dt = self.__keytransform__(start if start else datetime.now().date())
self._populate(dt.year - 1)
self._populate(dt.year + 1)
sorted_keys = sorted(self.keys())
pos = bisect_left(sorted_keys, dt) - 1 if previous else bisect_right(sorted_keys, dt)
if 0 <= pos < len(sorted_keys):
holiday_date = sorted_keys[pos]
holiday_name = self.get(holiday_date)
return holiday_date, holiday_name
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi,
I do not get your cornercase. UA has no holidays added after 2022-03-08, so nothing is returned. Whenever the calendar is changed again - and holidays will continue - the function returns one as well.
The function can only work with the current data set.
Also, do you really want to add another dependency for the expected rare occasions that this function is called?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry for wording it in a hard to understand way. Imagine a situation when holidays in 🇺🇦 are restored (also keep in mind there is a streak w/o holidays, right). So let's say 2025-01-01 was a holiday. If I run the function for 2022-03-08 date it'll return nothing while I expect (2025-01-01, Custom holiday)
The code:
if self._year >= 2025:
self._add_holiday_jan_1(tr("Custom holiday"))
The tests:
self.assertNotEqual(ua.get_next_holiday("2022-03-08"), (None, None)) # this fails
self.assertEqual(ua["2025-01-01"], "Custom holiday") # this passes
Please also note that tests in the reversed order will pass because of ua["2025-01-01"]
that populates 2025 year holidays.
a new function get_next_holiday is added to retrieve the date of the next known holiday. Also the name of the holiday is returned. It is possible to search forward and backward in time. This should solve vacanza#1825 Signed-off-by: Schrotti <[email protected]>
Use more stable calender for testing make sure calendar is filled also if no date is passed Signed-off-by: Schrotti <[email protected]>
677ac23
to
6fda9a8
Compare
Quality Gate passedIssues Measures |
Proposed change
a new function get_next_holiday is added to retrieve the date of the next known holiday.
Also the name of the holiday is returned.
It is possible to search forward and backward in time.
This should solve #1825
Documentation will be added once the basic changes are approved.
holidays
functionality in general)Checklist
make check
, all checks and tests are green