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.

Sign up for GitHub

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

Jump to bottom

Add support for more lookup types to pop_named (as in get_named method) #2140

Draft
wants to merge 2 commits into
base: dev
Choose a base branch
from
Draft

Add support for more lookup types to pop_named (as in get_named method) #2140

wants to merge 2 commits into from

Conversation

wth-d
Copy link

@wth-d wth-d commented Nov 26, 2024 •
edited
Loading

Proposed change

Adds the lookup parameter to the pop_named method
Addresses issue #2137

Type of change

  • 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

@codecov Codecov
Copy link

codecov bot commented Nov 26, 2024 •
edited
Loading

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 100.00%. Comparing base (93b3ca9) to head (a0a855d).
Report is 1 commits behind head on dev.

Additional details and impacted files 
@@            Coverage Diff            @@
##               dev     #2140   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files          194       194           
  Lines        11716     11716           
  Branches      1771      1771           
=========================================
  Hits         11716     11716

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

arkid15r
arkid15r reviewed Nov 26, 2024
View reviewed changes
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.

Hi @wth-d, thanks for addressing this! I'm really happy this feature request has found its implementer!

I understand it's still in a draft state, just though you'd like some early feedback on how to make your PR better:

holidays/holiday_base.py
@@ -1026,7 +1026,7 @@ def pop(self, key: DateLike, default: Union[str, Any] = None) -> Union[str, Any]

return dict.pop(self, self.__keytransform__(key), default)

def pop_named(self, name: str) -> list[date]:
def pop_named(self, name: str, lookup: str = "icontains") -> list[date]:
Copy link
Collaborator

Choose a reason for hiding this comment

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

Could you update the method's documentation with the new param too?

Copy link
Author

@wth-d wth-d Nov 27, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Changed the method's docstring
(the part for the lookup param is copied over from docstring of the get_named method)

holidays/holiday_base.py
@@ -1041,7 +1041,9 @@ def pop_named(self, name: str) -> list[date]:
KeyError if date is not a holiday and default is not given.
"""
use_exact_name = HOLIDAY_NAME_DELIMITER in name
if not (dts := self.get_named(name, split_multiple_names=not use_exact_name)):
if not (
dts := self.get_named(name, lookup=lookup, split_multiple_names=not use_exact_name)
Copy link
Collaborator

Choose a reason for hiding this comment

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

It'd be also great to have some tests added too

@wth-d
Copy link
Author

wth-d commented Nov 27, 2024

Hi @wth-d, thanks for addressing this! I'm really happy this feature request has found its implementer!

I understand it's still in a draft state, just though you'd like some early feedback on how to make your PR better:

Hi, thanks for your feedback and for posting this issue. I will try to address your feedback (it may have some delays).

@arkid15r arkid15r linked an issue Dec 2, 2024 that may be closed by this pull request
Extend HolidayBase::pop_named to support HolidayBase::get_named lookup param #2137
Open
@arkid15r
Copy link
Collaborator

Hi @wth-d, thanks for addressing this! I'm really happy this feature request has found its implementer!
I understand it's still in a draft state, just though you'd like some early feedback on how to make your PR better:

Hi, thanks for your feedback and for posting this issue. I will try to address your feedback (it may have some delays).

Hi @wth-d
Any progress w/ pop_named tests? Are you still interested in working on it or don't mind to offload it to the maintainers :) ?

@wth-d
Copy link
Author

wth-d commented Dec 28, 2024 •
edited
Loading

Hi @wth-d Any progress w/ pop_named tests? Are you still interested in working on it or don't mind to offload it to the maintainers :) ?

Hi, sorry I was busy in the last few weeks. I'm fine with offloading it to the maintainers now.

I've also got some draft of tests to add but haven't verified their correctness:

    def test_contains(self):
        self.assertIn("2022-01-01", self.hb)
        removed_dates = self.hb.pop_named("Day", lookup="contains")
        self.assertEqual(len(removed_dates), 6)
        self.assertNotIn("2022-01-01", self.hb)
        self.assertNotIn("2022-06-19", self.hb)
        self.assertNotIn("2022-06-20", self.hb)
        self.assertNotIn("2022-07-04", self.hb)
        self.assertNotIn("2022-12-25", self.hb)
        self.assertNotIn("2022-12-26", self.hb)

    def test_exact(self):
        self.assertIn("2022-01-01", self.hb)
        removed_dates = self.hb.pop_named("Presidents' Day", lookup="exact")
        self.assertEqual(len(removed_dates), 1)
        self.assertNotIn("2022-02-21", self.hb)

    def test_icontains(self):
        self.assertIn("2022-01-01", self.hb)
        removed_dates = self.hb.pop_named("day", lookup="icontains")
        self.assertEqual(len(removed_dates), 6)
        self.assertNotIn("2022-01-01", self.hb)
        self.assertNotIn("2022-06-19", self.hb)
        self.assertNotIn("2022-06-20", self.hb)
        self.assertNotIn("2022-07-04", self.hb)
        self.assertNotIn("2022-12-25", self.hb)
        self.assertNotIn("2022-12-26", self.hb)

A strange thing was that when I executed h.get_named("Day", lookup="contains", split_multiple_names=False) using the holidays library installed from pip, I got this as the result:

h = holidays.country_holidays('US')

[datetime.date(2022, 1, 1), datetime.date(2022, 5, 30), datetime.date(2022, 6, 19), 
datetime.date(2022, 6, 20), datetime.date(2022, 7, 4), datetime.date(2022, 9, 5), 
datetime.date(2022, 11, 11), datetime.date(2022, 12, 25), datetime.date(2022, 12, 26), 
datetime.date(2022, 1, 17)]

but when I executed it in the tests/test_holiday_base.py file (using self.hb instead of h), I got this:

[datetime.date(2022, 1, 1), datetime.date(2022, 6, 19), datetime.date(2022, 6, 20), 
datetime.date(2022, 7, 4), datetime.date(2022, 12, 25), datetime.date(2022, 12, 26)]

@KJhellico
Copy link
Collaborator

but when I executed it in the tests/test_holiday_base.py file (using self.hb instead of h), I got this:

self.hb is special CountryStub1 class, and it doesn't contain a full set of US holidays.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@arkid15r arkid15r arkid15r left review comments

@KJhellico KJhellico Awaiting requested review from KJhellico KJhellico will be requested when the pull request is marked ready for review KJhellico is a code owner

@PPsyrius PPsyrius Awaiting requested review from PPsyrius PPsyrius will be requested when the pull request is marked ready for review PPsyrius is a code owner

At least 1 approving review is required to merge this pull request.

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Extend HolidayBase::pop_named to support HolidayBase::get_named lookup param
3 participants
@wth-d @arkid15r @KJhellico