Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 100.00%. Comparing base (5e9f05a) to head (5f72812).
Report is 10 commits behind head on dev.

@@            Coverage Diff            @@
##               dev     #2140   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files          204       204           
  Lines        12999     13012   +13     
  Branches      1861      1866    +5     
=========================================
+ Hits         12999     13012   +13     

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

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

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

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.

We need to add handling of all lookup variants here, similar to get_named().

And in the case when we delete one holiday from multiple holidays date, should the method return this date?

And in the case when we delete one holiday from multiple holidays date, should the method return this date?

I think yes as long as there is another holiday on the same date.

But according to documentation, it returns list of dates removed. And this date has not been removed because there is still a holiday.

But according to documentation, it returns list of dates removed. And this date has not been removed because there is still a holiday.

Right, I forgot we talk about pop_named not get_named. This is something we need think through in order to minimize confusion from end users perspective.

I decide to bring this good idea to completion.

When deleting one holiday from multiple holidays date, this date will be present in function result (i.e., function behavior has not changed).

Summary by CodeRabbit

• New Features

  • Enhanced the holiday removal functionality by allowing users to specify flexible matching criteria, such as exact, partial, and case-sensitive or case-insensitive options.

• Tests

  • Expanded testing to validate multiple lookup strategies, ensuring that the updated holiday removal behaves as expected under various conditions.

Walkthrough

The changes update the pop_named method in the HolidayBase class by modifying its signature to accept a new holiday_name parameter and an optional lookup parameter, which allows for different matching strategies. The method logic, associated docstring, and exception handling were adjusted accordingly. Additionally, several new tests were added to verify the method's behavior using various lookup strategies such as exact, contains, and their case-sensitive variants.

Changes

Quality Gate passed

Issues
1 New issue
0 Accepted issues

Measures
0 Security Hotspots
0.0% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarQube Cloud

AFAIK this PR already got updated with Sphinx -> MkDocs migration, so it should be more or less ready

AFAIK this PR already got updated with Sphinx -> MkDocs migration, so it should be more or less ready

It's next in my queue for review