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

add function to get the next holiday #2211

Open
wants to merge 4 commits into
base: dev
Choose a base branch
from

Conversation

Rosi2143
Copy link
Contributor

@Rosi2143 Rosi2143 commented Jan 4, 2025

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.

  • New country/market holidays support (thank you!)
  • Supported country/market holidays update (calendar discrepancy fix, localization)
  • Existing code/documentation/test/process quality improvement (best practice, cleanup, refactoring, optimization)
  • Dependency update (version deprecation/pin/upgrade)
  • Bugfix (non-breaking change which fixes an issue)
  • Breaking change (a code change causing existing functionality to break)
  • New feature (new holidays functionality in general)

Checklist

  • I've followed the [contributing guidelines][contributing-guidelines]
  • I've successfully run make check, all checks and tests are green

Copy link

codecov bot commented Jan 4, 2025

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 100.00%. Comparing base (1a32771) to head (677ac23).

Additional details and impacted files
@@            Coverage Diff            @@
##               dev     #2211   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files          196       196           
  Lines        11842     11856   +14     
  Branches      1708      1712    +4     
=========================================
+ Hits         11842     11856   +14     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@KJhellico
Copy link
Collaborator

@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")

@Rosi2143
Copy link
Contributor Author

Rosi2143 commented Jan 4, 2025

@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.

@Rosi2143 Rosi2143 force-pushed the add_get_next_holiday branch from 053f942 to 562bfa1 Compare January 4, 2025 20:19
@github-actions github-actions bot added the doc label Jan 4, 2025
@Rosi2143 Rosi2143 force-pushed the add_get_next_holiday branch from 562bfa1 to c91e03e Compare January 4, 2025 21:07
Copy link
Collaborator

@KJhellico KJhellico left a 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.

docs/source/examples.rst Outdated Show resolved Hide resolved
holidays/holiday_base.py Outdated Show resolved Hide resolved
holidays/holiday_base.py Show resolved Hide resolved
@Rosi2143 Rosi2143 force-pushed the add_get_next_holiday branch from b353dde to a5554fb Compare January 9, 2025 20:36
Copy link
Collaborator

@KJhellico KJhellico left a 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! 👍

holidays/holiday_base.py Outdated Show resolved Hide resolved
holidays/holiday_base.py Outdated Show resolved Hide resolved
holidays/holiday_base.py Outdated Show resolved Hide resolved
holidays/holiday_base.py Outdated Show resolved Hide resolved
@Rosi2143 Rosi2143 force-pushed the add_get_next_holiday branch from a5554fb to c9cd832 Compare January 10, 2025 20:20
@Rosi2143 Rosi2143 force-pushed the add_get_next_holiday branch from c9cd832 to adf4653 Compare January 10, 2025 21:41
@KJhellico
Copy link
Collaborator

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

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 <Schrott.Micha@web.de>
@Rosi2143 Rosi2143 force-pushed the add_get_next_holiday branch from 8934029 to e56effc Compare January 12, 2025 11:44
@Rosi2143
Copy link
Contributor Author

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.
My guess was that this function will not be called frequently, so a little more execution time would not hurt.

But fixed this now as well.

holidays/holiday_base.py Outdated Show resolved Hide resolved
Rosi2143 and others added 3 commits January 13, 2025 19:18
Co-authored-by: ~Jhellico <KJhellico@users.noreply.github.com>
Signed-off-by: Rosi <Schrott.Micha@web.de>
self.assertEqual(ua.get_next_holiday("1991-01-01", previous=True), (None, None))

# check for date after end of calendar
self.assertEqual(ua.get_next_holiday("2022-03-08"), (None, None))
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's better to pick another country and date "2100-12-31" (or create special stub country). I hope the war will end soon, holidays will be restored in Ukraine, and then this test will fail.

Copy link
Collaborator

@arkid15r arkid15r left a 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:

(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()
Copy link
Collaborator

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.

@@ -959,6 +959,29 @@ def get_named(

raise AttributeError(f"Unknown lookup type: {lookup}")

def get_next_holiday(
Copy link
Collaborator

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?

@@ -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]]:
Copy link
Collaborator

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)?

Comment on lines +973 to +978
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)
Copy link
Collaborator

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.

Copy link
Collaborator

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

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

Successfully merging this pull request may close these issues.

3 participants