Skip to content

Refactor some HolidayBase methods#3079

Merged
arkid15r merged 3 commits intovacanza:devfrom
KJhellico:ref-holiday-base
Dec 1, 2025
Merged

Refactor some HolidayBase methods#3079
arkid15r merged 3 commits intovacanza:devfrom
KJhellico:ref-holiday-base

Conversation

@KJhellico
Copy link
Copy Markdown
Collaborator

Proposed change

Refactor some HolidayBase methods (__getitem__, __getstate__, get_named, pop_named).

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

@coderabbitai
Copy link
Copy Markdown
Contributor

coderabbitai bot commented Nov 26, 2025

Summary by CodeRabbit

Release Notes

  • Improvements
    • Type validation for holiday name lookup operations is now more robust, catching errors earlier
    • Range slice operations properly validate parameters and provide clearer error messages
    • Holiday object string representations are now more informative and consistent

✏️ Tip: You can customize this high-level summary in your review settings.

Walkthrough

Added a Literal type alias NameLookup and updated public method signatures to use it; refactored slice-based range lookup logic and repr formatting in HolidayBase; added a test ensuring non-integer/non-timedelta slice steps raise ValueError.

Changes

Cohort / File(s) Summary
Type alias & public API
holidays/holiday_base.py
Added NameLookup = Literal["contains","exact","startswith","icontains","iexact","istartswith"]. Updated get_named and pop_named signatures to use NameLookup for the lookup parameter.
Range-slicing logic
holidays/holiday_base.py
Reworked slice lookup: compute diff_days, adjust step sign, validate step types, and return matching days via a comprehension (uses walrus-assignment).
repr simplification
holidays/holiday_base.py
Simplified representation to return fully-formed strings directly based on market, country, and optional subdiv (or default to holidays.HolidayBase()).
Tests
tests/test_holiday_base.py
Added assertion in test_getitem verifying a non-integer/non-timedelta slice step (e.g., timedelta(hours=12)) raises ValueError.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~25 minutes

  • Inspect range-slicing changes for correct handling of int vs timedelta steps and sign logic.
  • Verify walrus-based comprehension preserves previous semantics and edge cases.
  • Confirm public signature change (NameLookup) is consistent with call sites and typing.

Possibly related PRs

Suggested reviewers

  • PPsyrius

Pre-merge checks and finishing touches

❌ Failed checks (1 warning)
Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 44.44% which is insufficient. The required threshold is 80.00%. You can run @coderabbitai generate docstrings to improve docstring coverage.
✅ Passed checks (2 passed)
Check name Status Explanation
Title check ✅ Passed The title accurately summarizes the main refactoring work across multiple HolidayBase methods, though it's intentionally broad rather than highlighting a single specific change.
Description check ✅ Passed The description clearly outlines the proposed changes (refactoring HolidayBase methods), specifies the type of change (quality improvement), and confirms contributor checklist compliance.
✨ Finishing touches
  • 📝 Generate docstrings
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Post copyable unit tests in a comment

📜 Recent review details

Configuration used: Path: .coderabbit.yaml

Review profile: ASSERTIVE

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between cdf6a19 and fc979ee.

📒 Files selected for processing (1)
  • holidays/holiday_base.py (5 hunks)
🧰 Additional context used
🧠 Learnings (11)
📓 Common learnings
Learnt from: KJhellico
Repo: vacanza/holidays PR: 2631
File: tests/countries/test_sint_maarten.py:62-0
Timestamp: 2025-06-14T21:12:07.224Z
Learning: KJhellico prefers to focus on completing and reviewing the main holiday implementation code before doing detailed reviews of the corresponding test files.
Learnt from: PPsyrius
Repo: vacanza/holidays PR: 2881
File: tests/countries/test_french_polynesia.py:19-22
Timestamp: 2025-11-08T05:09:56.159Z
Learning: In the vacanza/holidays project's test framework (PR #2881 onwards), the base class CommonCountryTests provides a default test_no_holidays implementation that automatically tests years=start_year - 1 for standard PUBLIC category cases. Individual country test files should only override test_no_holidays when the country supports additional non-PUBLIC categories (GOVERNMENT, WORKDAY, OPTIONAL, etc.) with different start years requiring separate validation.
Learnt from: KJhellico
Repo: vacanza/holidays PR: 2140
File: holidays/holiday_base.py:1202-1208
Timestamp: 2025-03-27T20:10:31.188Z
Learning: The `pop_named` method in HolidayBase should maintain its existing error handling behavior of raising a KeyError when no matching holidays are found, rather than returning an empty list.
Learnt from: PPsyrius
Repo: vacanza/holidays PR: 2881
File: tests/countries/test_albania.py:40-42
Timestamp: 2025-09-04T08:54:35.319Z
Learning: In the vacanza/holidays project test files, extract holiday name strings to local variables only when they are reused multiple times within the same test method (e.g., used in both assertHolidayName and assertNoHolidayName calls). When a holiday name is used only once, keep it inline rather than extracting it to a variable for the sake of consistency.
Learnt from: PPsyrius
Repo: vacanza/holidays PR: 2632
File: holidays/countries/solomon_islands.py:95-98
Timestamp: 2025-06-16T12:28:31.641Z
Learning: Library-wide holiday patterns and their optimizations should be handled at the base class level (like InternationalHolidays) rather than documenting workarounds in individual country modules. This maintains separation of concerns and avoids documentation duplication.
Learnt from: PPsyrius
Repo: vacanza/holidays PR: 2416
File: tests/countries/test_turkmenistan.py:52-64
Timestamp: 2025-04-05T04:47:27.213Z
Learning: For holiday tests in the vacanza/holidays project, structure tests by individual holidays rather than by years. Each test method should focus on a specific holiday and test it across multiple years (from start_year through 2050) using helper methods like `assertHolidayName`. For fixed holidays, use generators like `(f"{year}-01-01" for year in range(1991, 2051))`. For movable holidays, specify individual dates for specific years followed by a range check.
Learnt from: KJhellico
Repo: vacanza/holidays PR: 2608
File: tests/countries/test_saint_vincent_and_the_grenadines.py:162-178
Timestamp: 2025-07-02T18:21:59.302Z
Learning: In the Saint Vincent and the Grenadines holiday tests, for holidays without observed rules that only require a single assertHolidayName call, pass the holiday name directly as a string literal rather than storing it in a variable first for cleaner, more concise code.
Learnt from: PPsyrius
Repo: vacanza/holidays PR: 2881
File: tests/countries/test_argentina.py:375-375
Timestamp: 2025-09-03T16:49:35.246Z
Learning: In the holidays library, national holiday tests use self.full_range (or similar comprehensive year ranges) even when explicit test dates only show modern observance. This is intentional for correctness and comprehensive coverage, unlike subdivision-specific holidays which have explicit year boundaries based on known start dates.
Learnt from: PPsyrius
Repo: vacanza/holidays PR: 2490
File: holidays/countries/ethiopia.py:45-45
Timestamp: 2025-04-23T09:59:19.886Z
Learning: For the Ethiopia holidays class, it's appropriate to add a return type hint only to the `_is_leap_year` method to match the base class implementation in `holidays/holiday_base.py`, while keeping other methods without type hints to maintain consistency with other country implementations.
Learnt from: KJhellico
Repo: vacanza/holidays PR: 2653
File: holidays/locale/th/LC_MESSAGES/TW.po:17-21
Timestamp: 2025-06-21T18:06:50.027Z
Learning: KJhellico's username includes a tilde character (~) as part of their nickname (appears as "~Jhellico" in Last-Translator headers), which is intentional formatting and not an error.
📚 Learning: 2025-06-15T15:24:53.055Z
Learnt from: KJhellico
Repo: vacanza/holidays PR: 2606
File: holidays/countries/faroe_islands.py:62-67
Timestamp: 2025-06-15T15:24:53.055Z
Learning: The `HolidayBase` class uses `__getattr__` to dynamically implement `_add_holiday_*` methods through pattern matching, including patterns like `_add_holiday_<n>_days_past_easter`, `_add_holiday_<month>_<day>`, and various weekday-relative patterns. Methods like `_add_holiday_26_days_past_easter` are not explicitly defined but are dynamically generated when called.

Applied to files:

  • holidays/holiday_base.py
📚 Learning: 2025-10-28T17:26:45.090Z
Learnt from: PPsyrius
Repo: vacanza/holidays PR: 3026
File: holidays/countries/spain.py:189-189
Timestamp: 2025-10-28T17:26:45.090Z
Learning: In the vacanza/holidays codebase, `_populate` methods (such as `_populate_public_holidays`, `_populate_subdiv_holidays`, and subdivision-specific methods like `_populate_subdiv_XX_public_holidays`) intentionally do not include return type annotations like `-> None`. This is a consistent pattern across country implementations (e.g., Germany, United States, Spain) and should not be changed even when static analysis tools like Ruff suggest adding them.
<!-- </add_learning>

Applied to files:

  • holidays/holiday_base.py
📚 Learning: 2025-08-08T14:37:03.045Z
Learnt from: KJhellico
Repo: vacanza/holidays PR: 2774
File: tests/countries/test_liberia.py:15-16
Timestamp: 2025-08-08T14:37:03.045Z
Learning: In holidays/countries/__init__.py, re-export country classes using absolute imports (e.g., 'from holidays.countries.liberia import Liberia, LR, LBR') and keep alphabetical ordering (e.g., Lesotho, Liberia, Libya). Avoid relative imports in this file.

Applied to files:

  • holidays/holiday_base.py
📚 Learning: 2025-04-03T16:58:27.175Z
Learnt from: Wasif-Shahzad
Repo: vacanza/holidays PR: 2409
File: holidays/countries/qatar.py:27-46
Timestamp: 2025-04-03T16:58:27.175Z
Learning: In the holidays library, method names like `_add_holiday_2nd_tue_of_feb()` and `_add_holiday_1st_sun_of_mar()` use calendar constants internally through parent class implementations even when these constants don't appear directly in the file. Removing imports that seem unused based on a simple text search could break functionality.

Applied to files:

  • holidays/holiday_base.py
📚 Learning: 2025-04-23T09:59:19.886Z
Learnt from: PPsyrius
Repo: vacanza/holidays PR: 2490
File: holidays/countries/ethiopia.py:45-45
Timestamp: 2025-04-23T09:59:19.886Z
Learning: For the Ethiopia holidays class, it's appropriate to add a return type hint only to the `_is_leap_year` method to match the base class implementation in `holidays/holiday_base.py`, while keeping other methods without type hints to maintain consistency with other country implementations.

Applied to files:

  • holidays/holiday_base.py
📚 Learning: 2025-03-27T20:10:31.188Z
Learnt from: KJhellico
Repo: vacanza/holidays PR: 2140
File: holidays/holiday_base.py:1202-1208
Timestamp: 2025-03-27T20:10:31.188Z
Learning: The `pop_named` method in HolidayBase should maintain its existing error handling behavior of raising a KeyError when no matching holidays are found, rather than returning an empty list.

Applied to files:

  • holidays/holiday_base.py
📚 Learning: 2025-06-16T12:28:31.641Z
Learnt from: PPsyrius
Repo: vacanza/holidays PR: 2632
File: holidays/countries/solomon_islands.py:95-98
Timestamp: 2025-06-16T12:28:31.641Z
Learning: Library-wide holiday patterns and their optimizations should be handled at the base class level (like InternationalHolidays) rather than documenting workarounds in individual country modules. This maintains separation of concerns and avoids documentation duplication.

Applied to files:

  • holidays/holiday_base.py
📚 Learning: 2025-09-17T15:16:16.192Z
Learnt from: KJhellico
Repo: vacanza/holidays PR: 2944
File: holidays/countries/myanmar.py:183-191
Timestamp: 2025-09-17T15:16:16.192Z
Learning: The special_public_holidays formatting pattern in the holidays codebase follows a consistent convention: single entries per year use flat tuple format like `2024: (MONTH, DAY, name)`, while multiple entries per year use tuple-of-tuples format like `2024: ((MONTH, DAY, name), (MONTH, DAY, name))`. This pattern is used consistently across all countries.

Applied to files:

  • holidays/holiday_base.py
📚 Learning: 2025-09-18T07:01:12.236Z
Learnt from: PPsyrius
Repo: vacanza/holidays PR: 2942
File: holidays/countries/south_africa.py:91-98
Timestamp: 2025-09-18T07:01:12.236Z
Learning: In the holidays library South Africa implementation, inline ternary operators for holiday name selection (like "Republic Day" if self._year >= 1961 else "Union Day") are intentionally kept inline rather than extracted to separate variables, as this structure is designed in preparation for future localization (l10n) support where the contextual relationship between conditions and translatable strings needs to be preserved.

Applied to files:

  • holidays/holiday_base.py
📚 Learning: 2025-03-30T20:18:46.006Z
Learnt from: KJhellico
Repo: vacanza/holidays PR: 2386
File: holidays/countries/nepal.py:24-26
Timestamp: 2025-03-30T20:18:46.006Z
Learning: In the holidays library, country classes do not directly implement `_populate()`. Instead, they implement specialized methods like `_populate_public_holidays()`, and the base class `HolidayBase` handles the orchestration by calling these specialized methods.

Applied to files:

  • holidays/holiday_base.py
🧬 Code graph analysis (1)
holidays/holiday_base.py (1)
holidays/calendars/gregorian.py (1)
  • _timedelta (37-42)
🪛 Ruff (0.14.6)
holidays/holiday_base.py

567-567: Avoid specifying long messages outside the exception class

(TRY003)


570-570: Avoid specifying long messages outside the exception class

(TRY003)


997-997: Boolean-typed positional argument in function definition

(FBT001)


997-997: Boolean default positional argument in function definition

(FBT002)

⏰ Context from checks skipped due to timeout of 300000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (5)
  • GitHub Check: Test Python 3.10 on windows-latest
  • GitHub Check: Test Python 3.12 on windows-latest
  • GitHub Check: Test Python 3.13 on windows-latest
  • GitHub Check: Test Python 3.14 on windows-latest
  • GitHub Check: Test Python 3.11 on windows-latest
🔇 Additional comments (2)
holidays/holiday_base.py (2)

668-680: New __repr__ paths neatly mirror the public factories

The empty-case __repr__ now returning holidays.financial_holidays(<market>), holidays.country_holidays(<country>, subdiv=...), or holidays.HolidayBase() is concise and aligns with how users construct these instances. The guard to fall back to the dict __repr__ when self is non-empty preserves existing behavior. No issues here.


1202-1274: Guard pop_named against duplicate dates returned by get_named

Queries like "Day" with lookup="icontains" can return the same date multiple times when multiple holiday names on that date match the search. This causes the method to call self[dt] after self.pop(dt) has already removed the date, raising KeyError on the second occurrence.

Deduplicate dates before iterating to prevent this:

-        if not (
-            dts := self.get_named(
-                holiday_name, lookup=lookup, split_multiple_names=not use_exact_name
-            )
-        ):
+        if not (
+            dts := list(
+                dict.fromkeys(
+                    self.get_named(
+                        holiday_name,
+                        lookup=lookup,
+                        split_multiple_names=not use_exact_name,
+                    )
+                )
+            )
+        ):
             raise KeyError(holiday_name)

This preserves ordering and maintains the existing KeyError contract when no holidays match.


Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

@codecov
Copy link
Copy Markdown

codecov bot commented Nov 26, 2025

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 100.00%. Comparing base (f1de653) to head (752e313).
⚠️ Report is 3 commits behind head on dev.

Additional details and impacted files
@@            Coverage Diff            @@
##               dev     #3079   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files          306       306           
  Lines        18108     18100    -8     
  Branches      2309      2307    -2     
=========================================
- Hits         18108     18100    -8     

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

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

PPsyrius
PPsyrius previously approved these changes Nov 26, 2025
Copy link
Copy Markdown
Collaborator

@PPsyrius PPsyrius left a comment

Choose a reason for hiding this comment

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

LGTM 🛠️

@arkid15r arkid15r enabled auto-merge December 1, 2025 18:03
@sonarqubecloud
Copy link
Copy Markdown

sonarqubecloud bot commented Dec 1, 2025

@arkid15r arkid15r added this pull request to the merge queue Dec 1, 2025
Merged via the queue into vacanza:dev with commit 8da43dd Dec 1, 2025
33 checks passed
@KJhellico KJhellico deleted the ref-holiday-base branch December 1, 2025 19:00
@arkid15r arkid15r mentioned this pull request Dec 1, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants