Long weekend Function added #3001
Conversation
Summary by CodeRabbit
WalkthroughAdds a new public utility function Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Potential attention points:
Suggested reviewers
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro 📒 Files selected for processing (2)
🧰 Additional context used🧠 Learnings (7)📚 Learning: 2025-09-14T17:17:14.387ZApplied to files:
📚 Learning: 2025-04-05T04:47:27.213ZApplied to files:
📚 Learning: 2025-03-23T16:59:25.794ZApplied to files:
📚 Learning: 2025-04-05T04:29:38.042ZApplied to files:
📚 Learning: 2025-04-05T06:49:06.217ZApplied to files:
📚 Learning: 2025-09-16T09:44:45.875ZApplied to files:
📚 Learning: 2025-09-04T08:54:35.319ZApplied to files:
🧬 Code graph analysis (2)tests/test_utils.py (2)
holidays/utils.py (2)
🪛 Ruff (0.14.1)tests/test_utils.py227-227: Use a regular Replace (PT009) 231-231: Missing return type annotation for special method Add return type annotation: (ANN204) 231-231: Do not use mutable data structures for argument defaults Replace with (B006) 239-239: Missing return type annotation for special method Add return type annotation: (ANN204) 248-248: Use a regular Replace (PT009) 258-258: Use a regular Replace (PT009) 264-264: Use a regular Replace (PT009) 270-270: Use a regular Replace (PT009) 276-276: Use a regular Replace (PT009) 281-281: Use a regular Replace (PT009) 287-287: Use a regular Replace (PT009) 293-293: Use a regular Replace (PT009) holidays/utils.py445-445: Boolean-typed positional argument in function definition (FBT001) 445-445: Boolean default positional argument in function definition (FBT002) 🔇 Additional comments (3)
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. Comment |
There was a problem hiding this comment.
Actionable comments posted: 3
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (2)
holidays/utils.py(2 hunks)tests/test_utils.py(2 hunks)
🧰 Additional context used
🧠 Learnings (4)
📚 Learning: 2025-04-05T06:49:06.217Z
Learnt from: PPsyrius
PR: vacanza/holidays#2386
File: tests/countries/test_nepal.py:499-536
Timestamp: 2025-04-05T06:49:06.217Z
Learning: In the holidays project, test files follow a dual testing approach: individual methods test specific holidays across multiple years, while comprehensive year-specific tests (e.g., `test_2025`) verify all holidays for a specific year in a single assertion. Both approaches serve different testing purposes and complement each other.
Applied to files:
tests/test_utils.py
📚 Learning: 2025-08-12T17:16:54.497Z
Learnt from: PPsyrius
PR: vacanza/holidays#2794
File: tests/calendars/test_julian.py:35-36
Timestamp: 2025-08-12T17:16:54.497Z
Learning: In the vacanza/holidays project calendar tests (Thai, Ethiopian, Julian, etc.), the established testing pattern for validation methods is to use simple for loops like `for year in known_data_dict:` followed by `self.assertEqual(expected, actual)` without using unittest's subTest feature. This pattern is consistently maintained across all calendar test files.
Applied to files:
tests/test_utils.py
📚 Learning: 2025-04-05T04:47:27.213Z
Learnt from: PPsyrius
PR: vacanza/holidays#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.
Applied to files:
tests/test_utils.py
📚 Learning: 2025-09-10T14:35:54.603Z
Learnt from: PPsyrius
PR: vacanza/holidays#2881
File: tests/countries/test_brazil.py:28-30
Timestamp: 2025-09-10T14:35:54.603Z
Learning: In the holidays project, the test_no_holidays method should test ALL supported_categories (via categories=CountryClass.supported_categories) rather than just the default PUBLIC category, to ensure comprehensive validation that no holidays exist before start_year across any supported category including OPTIONAL and subdivision categories.
Applied to files:
tests/test_utils.py
🧬 Code graph analysis (2)
holidays/utils.py (1)
holidays/holiday_base.py (1)
get_nth_working_day(1092-1116)
tests/test_utils.py (1)
holidays/utils.py (1)
list_long_weekends(442-487)
🪛 Ruff (0.13.3)
tests/test_utils.py
226-226: Use a regular assert instead of unittest-style assertEqual
Replace assertEqual(...) with assert ...
(PT009)
232-232: Use a regular assert instead of unittest-style assertIsInstance
Replace assertIsInstance(...) with assert ...
(PT009)
233-233: Use a regular assert instead of unittest-style assertTrue
Replace assertTrue(...) with assert ...
(PT009)
237-237: Use a regular assert instead of unittest-style assertEqual
Replace assertEqual(...) with assert ...
(PT009)
241-241: Use a regular assert instead of unittest-style assertIsInstance
Replace assertIsInstance(...) with assert ...
(PT009)
246-246: Use a regular assert instead of unittest-style assertIsInstance
Replace assertIsInstance(...) with assert ...
(PT009)
247-247: Use a regular assert instead of unittest-style assertIsInstance
Replace assertIsInstance(...) with assert ...
(PT009)
248-248: Use a regular assert instead of unittest-style assertIsInstance
Replace assertIsInstance(...) with assert ...
(PT009)
252-252: Use a regular assert instead of unittest-style assertIsInstance
Replace assertIsInstance(...) with assert ...
(PT009)
🔇 Additional comments (1)
holidays/utils.py (1)
21-22: Public API export looks goodAdding list_long_weekends to all aligns with existing naming (list_*).
| financial_files = [ | ||
| path for path in Path("holidays/financial").glob("*.py") if path.stem != "__init__" | ||
| ] | ||
| self.assertEqual(len(financial_files), len(supported_financial)) | ||
|
|
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Redundant parity check; remove duplicate assertion
You already assert parity at Lines 219-222. Lines 223-227 repeat the same check. Trim the duplicate to keep tests lean.
- financial_files = [
- path for path in Path("holidays/financial").glob("*.py") if path.stem != "__init__"
- ]
- self.assertEqual(len(financial_files), len(supported_financial))
-📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| financial_files = [ | |
| path for path in Path("holidays/financial").glob("*.py") if path.stem != "__init__" | |
| ] | |
| self.assertEqual(len(financial_files), len(supported_financial)) |
🧰 Tools
🪛 Ruff (0.13.3)
226-226: Use a regular assert instead of unittest-style assertEqual
Replace assertEqual(...) with assert ...
(PT009)
🤖 Prompt for AI Agents
In tests/test_utils.py around lines 223 to 227, there's a duplicate parity
assertion that repeats the check already made at lines 219-222; remove the
redundant block (lines 223-227) so only the original parity assertion remains,
keeping the test lean and avoiding duplicate checks.
There was a problem hiding this comment.
As the bot says, this section is currently duplicate of the refactored tests above and should be removed
| financial_files = [ | |
| path for path in Path("holidays/financial").glob("*.py") if path.stem != "__init__" | |
| ] | |
| self.assertEqual(len(financial_files), len(supported_financial)) |
There was a problem hiding this comment.
@PPsyrius, understood! The duplicate assertion in lines 223-227 should indeed be removed since it repeats the same parity check already performed at lines 219-222. Both count the financial files and verify they match supported_financial, making the second one redundant.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## dev #3001 +/- ##
=========================================
Coverage 100.00% 100.00%
=========================================
Files 306 306
Lines 18062 18085 +23
Branches 2333 2339 +6
=========================================
+ Hits 18062 18085 +23 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Actionable comments posted: 6
♻️ Duplicate comments (2)
tests/test_utils.py (2)
224-228: Remove duplicate parity assertion (already checked above).This repeats the parity check done at Lines 220-223. Trim to avoid double work.
- financial_files = [ - path for path in Path("holidays/financial").glob("*.py") if path.stem != "__init__" - ] - self.assertEqual(len(financial_files), len(supported_financial)) -
244-249: Add a cross‑year boundary test (New Year’s).Covers spans that start in prior year. Mirrors the feature’s doc intent.
class TestListLongWeekends(unittest.TestCase): @@ def test_no_holidays(self): instance = MockHolidayBase([]) result = list_long_weekends(instance) self.assertEqual(result, []) + + def test_cross_year_boundary(self): + # US 2024: New Year's Day on Monday yields Sat 2023‑12‑30 to Mon 2024‑01‑01. + us = holidays.country_holidays("US", years=2024) + result = list_long_weekends(us) + self.assertTrue(any(start.year == 2023 and end.year == 2024 for start, end in result))Based on learnings
Also applies to: 250-259, 260-271, 272-278
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (2)
holidays/utils.py(2 hunks)tests/test_utils.py(3 hunks)
🧰 Additional context used
🧠 Learnings (3)
📚 Learning: 2025-09-14T17:17:14.387Z
Learnt from: PPsyrius
PR: vacanza/holidays#2881
File: tests/countries/test_saint_helena_ascension_and_tristan_da_cunha.py:209-209
Timestamp: 2025-09-14T17:17:14.387Z
Learning: In tests/countries/test_saint_helena_ascension_and_tristan_da_cunha.py, the explicit loop iteration pattern for subdivision-specific holiday checks (like Anniversary Day for TA subdivision) is intentionally preferred over using assertSubdivTa helper methods, as confirmed by PPsyrius.
Applied to files:
tests/test_utils.py
📚 Learning: 2025-04-05T04:47:27.213Z
Learnt from: PPsyrius
PR: vacanza/holidays#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.
Applied to files:
tests/test_utils.py
📚 Learning: 2025-04-05T04:29:38.042Z
Learnt from: PPsyrius
PR: vacanza/holidays#2416
File: tests/countries/test_turkmenistan.py:85-86
Timestamp: 2025-04-05T04:29:38.042Z
Learning: For testing holiday implementations in the vacanza/holidays repository, recommend using `from tests.common import CommonCountryTests` as the base class instead of directly using `unittest.TestCase` to maintain consistency with project conventions and leverage common test utilities.
Applied to files:
tests/test_utils.py
🧬 Code graph analysis (2)
tests/test_utils.py (2)
holidays/holiday_base.py (1)
HolidayBase(57-1304)holidays/utils.py (1)
list_long_weekends(442-477)
holidays/utils.py (1)
holidays/holiday_base.py (1)
get_nth_working_day(1099-1123)
🪛 Ruff (0.14.0)
tests/test_utils.py
227-227: Use a regular assert instead of unittest-style assertEqual
Replace assertEqual(...) with assert ...
(PT009)
231-231: Missing return type annotation for special method __init__
Add return type annotation: None
(ANN204)
231-231: Do not use mutable data structures for argument defaults
Replace with None; initialize within function
(B006)
239-239: Missing return type annotation for special method __contains__
Add return type annotation: bool
(ANN204)
248-248: Use a regular assert instead of unittest-style assertEqual
Replace assertEqual(...) with assert ...
(PT009)
258-258: Use a regular assert instead of unittest-style assertEqual
Replace assertEqual(...) with assert ...
(PT009)
264-264: Use a regular assert instead of unittest-style assertEqual
Replace assertEqual(...) with assert ...
(PT009)
270-270: Use a regular assert instead of unittest-style assertEqual
Replace assertEqual(...) with assert ...
(PT009)
276-276: Use a regular assert instead of unittest-style assertTrue
Replace assertTrue(...) with assert ...
(PT009)
277-277: Use a regular assert instead of unittest-style assertTrue
Replace assertTrue(...) with assert ...
(PT009)
282-282: Use a regular assert instead of unittest-style assertEqual
Replace assertEqual(...) with assert ...
(PT009)
holidays/utils.py
444-444: Boolean-typed positional argument in function definition
(FBT001)
444-444: Boolean default positional argument in function definition
(FBT002)
🔇 Additional comments (2)
tests/test_utils.py (1)
279-282: LGTM: empty input returns empty list.holidays/utils.py (1)
21-21: Public API export looks good.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
tests/test_utils.py (2)
224-228: Remove the duplicate parity assertion.This repeats the same check from lines 220-223. One assertion is enough.
- financial_files = [ - path for path in Path("holidays/financial").glob("*.py") if path.stem != "__init__" - ] - self.assertEqual(len(financial_files), len(supported_financial)) -
230-234: Fix the mutable default argument.The
weekend={5, 6}default is mutable (Ruff B006). Replace withNoneand initialize inside the method:- def __init__(self, holidays, weekend={5, 6}): + def __init__(self, holidays, weekend=None): super().__init__() self._holidays = set(holidays) - self.weekend = weekend + self.weekend = weekend if weekend is not None else {5, 6}Optionally, add type hints for clarity:
- def __init__(self, holidays, weekend=None): + def __init__(self, holidays, weekend=None) -> None:
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (2)
holidays/utils.py(2 hunks)tests/test_utils.py(3 hunks)
🧰 Additional context used
🧠 Learnings (4)
📚 Learning: 2025-09-14T17:17:14.387Z
Learnt from: PPsyrius
PR: vacanza/holidays#2881
File: tests/countries/test_saint_helena_ascension_and_tristan_da_cunha.py:209-209
Timestamp: 2025-09-14T17:17:14.387Z
Learning: In tests/countries/test_saint_helena_ascension_and_tristan_da_cunha.py, the explicit loop iteration pattern for subdivision-specific holiday checks (like Anniversary Day for TA subdivision) is intentionally preferred over using assertSubdivTa helper methods, as confirmed by PPsyrius.
Applied to files:
tests/test_utils.py
📚 Learning: 2025-04-05T04:47:27.213Z
Learnt from: PPsyrius
PR: vacanza/holidays#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.
Applied to files:
tests/test_utils.py
📚 Learning: 2025-03-23T16:59:25.794Z
Learnt from: PPsyrius
PR: vacanza/holidays#2362
File: tests/test_ical.py:31-40
Timestamp: 2025-03-23T16:59:25.794Z
Learning: In the holidays library's `TestIcalExporter` class, initializing `ICalExporter` with `None` is intentional. Using a mock object instead breaks tests because the constructor performs attribute validation with `getattr(self.holidays, "language", None)` which returns a mock with a mock object instead of `None`, causing validation failures. The approach isolates file operation tests from iCal generation logic.
Applied to files:
tests/test_utils.py
📚 Learning: 2025-04-05T04:29:38.042Z
Learnt from: PPsyrius
PR: vacanza/holidays#2416
File: tests/countries/test_turkmenistan.py:85-86
Timestamp: 2025-04-05T04:29:38.042Z
Learning: For testing holiday implementations in the vacanza/holidays repository, recommend using `from tests.common import CommonCountryTests` as the base class instead of directly using `unittest.TestCase` to maintain consistency with project conventions and leverage common test utilities.
Applied to files:
tests/test_utils.py
🧬 Code graph analysis (2)
tests/test_utils.py (2)
holidays/holiday_base.py (1)
HolidayBase(57-1304)holidays/utils.py (1)
list_long_weekends(442-480)
holidays/utils.py (1)
holidays/holiday_base.py (2)
get_nth_working_day(1099-1123)is_weekend(1148-1158)
🪛 Ruff (0.14.0)
tests/test_utils.py
227-227: Use a regular assert instead of unittest-style assertEqual
Replace assertEqual(...) with assert ...
(PT009)
231-231: Missing return type annotation for special method __init__
Add return type annotation: None
(ANN204)
231-231: Do not use mutable data structures for argument defaults
Replace with None; initialize within function
(B006)
239-239: Missing return type annotation for special method __contains__
Add return type annotation: bool
(ANN204)
248-248: Use a regular assert instead of unittest-style assertEqual
Replace assertEqual(...) with assert ...
(PT009)
258-258: Use a regular assert instead of unittest-style assertEqual
Replace assertEqual(...) with assert ...
(PT009)
264-264: Use a regular assert instead of unittest-style assertEqual
Replace assertEqual(...) with assert ...
(PT009)
270-270: Use a regular assert instead of unittest-style assertEqual
Replace assertEqual(...) with assert ...
(PT009)
276-276: Use a regular assert instead of unittest-style assertEqual
Replace assertEqual(...) with assert ...
(PT009)
281-281: Use a regular assert instead of unittest-style assertEqual
Replace assertEqual(...) with assert ...
(PT009)
holidays/utils.py
444-444: Boolean-typed positional argument in function definition
(FBT001)
444-444: Boolean default positional argument in function definition
(FBT002)
🔇 Additional comments (1)
tests/test_utils.py (1)
243-281: Good test coverage of core scenarios.The tests cover the main cases well: basic long weekends, multiple holidays, the inclusion flag, custom weekends, and empty input.
Consider adding an edge case test for long weekends that span year boundaries (e.g., Dec 31 + Jan 1) if you want even more thorough coverage, but what's here is solid.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
holidays/utils.py (1)
442-446: Fix the function signature: importdate, use plainbool, make parameter keyword-only.Three issues here:
- Critical: The return type should be
list[list[date]], butdateisn't imported. Add it to line 26.Optional[bool]is redundant—boolcan't beNone. Usebooldirectly.- The boolean parameter should be keyword-only to prevent accidental positional usage.
Apply these changes:
-from datetime import timedelta +from datetime import date, timedeltadef list_long_weekends( instance: HolidayBase, + *, minimum_holiday_length: int = 3, - require_weekend_overlap: Optional[bool] = True, + require_weekend_overlap: bool = True, -) -> list: +) -> list[list[date]]:tests/test_utils.py (1)
243-281: Consider strengthening the test suite with invariant checks and edge cases.The current tests validate the happy path well. Adding a few defensive checks would make the suite more robust:
- Invariant assertion: Verify that all returned blocks satisfy
length >= minimum_holiday_length(currently hardcoded as 3 in tests).- Cross-year boundary: Add a test where a long weekend spans Dec 31 → Jan 1 to confirm the function handles year transitions correctly.
- Invalid subdivision: Mirror the
test_invalid_country_subdivision_raises_not_implementedpattern fromTestCountryHolidaysif applicable.Example additions:
def test_minimum_length_invariant(self): holidays = [date(2025, 7, 4), date(2025, 12, 26)] instance = MockHolidayBase(holidays) result = list_long_weekends(instance) # Verify all blocks are >= 3 days self.assertTrue(all(len(block) >= 3 for block in result)) def test_cross_year_boundary(self): # Jan 1, 2024 (Monday) → weekend Sat-Sun Dec 30-31, 2023 holidays = [date(2024, 1, 1)] instance = MockHolidayBase(holidays) result = list_long_weekends(instance) # Verify at least one block spans years self.assertTrue(any(block[0].year != block[-1].year for block in result if block))
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (2)
holidays/utils.py(2 hunks)tests/test_utils.py(3 hunks)
🧰 Additional context used
🧠 Learnings (6)
📚 Learning: 2025-09-16T09:44:45.875Z
Learnt from: PPsyrius
PR: vacanza/holidays#2881
File: holidays/countries/hungary.py:46-49
Timestamp: 2025-09-16T09:44:45.875Z
Learning: In the holidays library, using generic variable names like `name` for holiday names within country implementations is acceptable and follows established patterns across other country files, even when the same variable name might be reused for different holidays within the same method.
Applied to files:
holidays/utils.py
📚 Learning: 2025-09-04T08:54:35.319Z
Learnt from: PPsyrius
PR: vacanza/holidays#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.
Applied to files:
holidays/utils.py
📚 Learning: 2025-09-14T17:17:14.387Z
Learnt from: PPsyrius
PR: vacanza/holidays#2881
File: tests/countries/test_saint_helena_ascension_and_tristan_da_cunha.py:209-209
Timestamp: 2025-09-14T17:17:14.387Z
Learning: In tests/countries/test_saint_helena_ascension_and_tristan_da_cunha.py, the explicit loop iteration pattern for subdivision-specific holiday checks (like Anniversary Day for TA subdivision) is intentionally preferred over using assertSubdivTa helper methods, as confirmed by PPsyrius.
Applied to files:
tests/test_utils.py
📚 Learning: 2025-04-05T04:47:27.213Z
Learnt from: PPsyrius
PR: vacanza/holidays#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.
Applied to files:
tests/test_utils.py
📚 Learning: 2025-03-23T16:59:25.794Z
Learnt from: PPsyrius
PR: vacanza/holidays#2362
File: tests/test_ical.py:31-40
Timestamp: 2025-03-23T16:59:25.794Z
Learning: In the holidays library's `TestIcalExporter` class, initializing `ICalExporter` with `None` is intentional. Using a mock object instead breaks tests because the constructor performs attribute validation with `getattr(self.holidays, "language", None)` which returns a mock with a mock object instead of `None`, causing validation failures. The approach isolates file operation tests from iCal generation logic.
Applied to files:
tests/test_utils.py
📚 Learning: 2025-04-05T04:29:38.042Z
Learnt from: PPsyrius
PR: vacanza/holidays#2416
File: tests/countries/test_turkmenistan.py:85-86
Timestamp: 2025-04-05T04:29:38.042Z
Learning: For testing holiday implementations in the vacanza/holidays repository, recommend using `from tests.common import CommonCountryTests` as the base class instead of directly using `unittest.TestCase` to maintain consistency with project conventions and leverage common test utilities.
Applied to files:
tests/test_utils.py
🧬 Code graph analysis (2)
holidays/utils.py (1)
holidays/holiday_base.py (2)
get_nth_working_day(1099-1123)is_weekend(1148-1158)
tests/test_utils.py (2)
holidays/holiday_base.py (1)
HolidayBase(57-1304)holidays/utils.py (1)
list_long_weekends(442-500)
🪛 Ruff (0.14.1)
holidays/utils.py
445-445: Boolean-typed positional argument in function definition
(FBT001)
445-445: Boolean default positional argument in function definition
(FBT002)
tests/test_utils.py
227-227: Use a regular assert instead of unittest-style assertEqual
Replace assertEqual(...) with assert ...
(PT009)
231-231: Missing return type annotation for special method __init__
Add return type annotation: None
(ANN204)
231-231: Do not use mutable data structures for argument defaults
Replace with None; initialize within function
(B006)
239-239: Missing return type annotation for special method __contains__
Add return type annotation: bool
(ANN204)
248-248: Use a regular assert instead of unittest-style assertEqual
Replace assertEqual(...) with assert ...
(PT009)
258-258: Use a regular assert instead of unittest-style assertEqual
Replace assertEqual(...) with assert ...
(PT009)
264-264: Use a regular assert instead of unittest-style assertEqual
Replace assertEqual(...) with assert ...
(PT009)
270-270: Use a regular assert instead of unittest-style assertEqual
Replace assertEqual(...) with assert ...
(PT009)
276-276: Use a regular assert instead of unittest-style assertEqual
Replace assertEqual(...) with assert ...
(PT009)
281-281: Use a regular assert instead of unittest-style assertEqual
Replace assertEqual(...) with assert ...
(PT009)
| require_weekend_overlap: | ||
| Whether to include consecutive holidays that do not contain any weekend days. | ||
| Defaults to True. |
There was a problem hiding this comment.
Param description seems reversed.
There was a problem hiding this comment.
Since the param name is changed I changed its default value to True and made changes to the code wherever required.
require_weekend_overlap makes more sense with its default value being True.
There was a problem hiding this comment.
I mean that the description "Whether to include consecutive holidays that do not contain any weekend days" can be understood as "If True, include consecutive holidays that do not contain..."
There was a problem hiding this comment.
| require_weekend_overlap: | |
| Whether to include consecutive holidays that do not contain any weekend days. | |
| Defaults to True. | |
| require_weekend_overlap: | |
| Whether to include only consecutive holidays that overlap with a weekend. | |
| Defaults to True. |
|
| require_weekend_overlap: | ||
| Whether to include consecutive holidays that do not contain any weekend days. | ||
| Defaults to True. |
There was a problem hiding this comment.
| require_weekend_overlap: | |
| Whether to include consecutive holidays that do not contain any weekend days. | |
| Defaults to True. | |
| require_weekend_overlap: | |
| Whether to include only consecutive holidays that overlap with a weekend. | |
| Defaults to True. |
| financial_files = [ | ||
| path for path in Path("holidays/financial").glob("*.py") if path.stem != "__init__" | ||
| ] | ||
| self.assertEqual(len(financial_files), len(supported_financial)) | ||
|
|
There was a problem hiding this comment.
As the bot says, this section is currently duplicate of the refactored tests above and should be removed
| financial_files = [ | |
| path for path in Path("holidays/financial").glob("*.py") if path.stem != "__init__" | |
| ] | |
| self.assertEqual(len(financial_files), len(supported_financial)) |
| import pytest | ||
|
|
||
| import holidays | ||
| from holidays.holiday_base import HolidayBase |
There was a problem hiding this comment.
| from holidays.holiday_base import HolidayBase | |
| from holidays.calendars.gregorian import FRI, SAT, SUN | |
| from holidays.holiday_base import HolidayBase |
| def __init__(self, holidays, weekend={5, 6}): | ||
| super().__init__() | ||
| self._holidays = set(holidays) | ||
| self.weekend = weekend |
There was a problem hiding this comment.
| def __init__(self, holidays, weekend={5, 6}): | |
| super().__init__() | |
| self._holidays = set(holidays) | |
| self.weekend = weekend | |
| def __init__(self, holidays: set[date], weekend: set[int] | None = None) -> None: | |
| super().__init__() | |
| self._holidays = set(holidays) | |
| self.weekend = weekend or {SAT, SUN} |
There was a problem hiding this comment.
I would apply the same approach here as in test_holiday_base - country stubs with the necessary sets of holidays and weekends.
| def __contains__(self, d): | ||
| return d in self._holidays |
There was a problem hiding this comment.
| def __contains__(self, d): | |
| return d in self._holidays | |
| def __contains__(self, day): | |
| return day in self._holidays |
| class TestListLongWeekends(unittest.TestCase): | ||
| def test_simple_long_weekend(self): | ||
| holidays = [date(2025, 7, 4)] | ||
| instance = MockHolidayBase(holidays) | ||
| result = list_long_weekends(instance) | ||
| self.assertEqual(result, [[date(2025, 7, 4), date(2025, 7, 5), date(2025, 7, 6)]]) | ||
|
|
||
| def test_multiple_holidays(self): | ||
| holidays = [date(2025, 7, 4), date(2025, 12, 26)] | ||
| instance = MockHolidayBase(holidays) | ||
| result = list_long_weekends(instance) | ||
| expected = [ | ||
| [date(2025, 7, 4), date(2025, 7, 5), date(2025, 7, 6)], | ||
| [date(2025, 12, 26), date(2025, 12, 27), date(2025, 12, 28)], | ||
| ] | ||
| self.assertEqual(result, expected) | ||
|
|
||
| def test_three_holidays_no_weekend(self): | ||
| holidays = [date(2025, 3, 4), date(2025, 3, 5), date(2025, 3, 6)] | ||
| instance = MockHolidayBase(holidays) | ||
| result = list_long_weekends(instance) | ||
| self.assertEqual(result, []) | ||
|
|
||
| def test_three_holidays_included_with_flag(self): | ||
| holidays = [date(2025, 3, 4), date(2025, 3, 5), date(2025, 3, 6)] | ||
| instance = MockHolidayBase(holidays) | ||
| result = list_long_weekends(instance, require_weekend_overlap=False) | ||
| self.assertEqual(result, [[date(2025, 3, 4), date(2025, 3, 5), date(2025, 3, 6)]]) | ||
|
|
||
| def test_custom_weekend(self): | ||
| holidays = [date(2025, 4, 10)] | ||
| instance = MockHolidayBase(holidays, weekend={4, 5}) | ||
| result = list_long_weekends(instance) | ||
| self.assertEqual(result, [[date(2025, 4, 10), date(2025, 4, 11), date(2025, 4, 12)]]) | ||
|
|
||
| def test_no_holidays(self): | ||
| instance = MockHolidayBase([]) | ||
| result = list_long_weekends(instance) | ||
| self.assertEqual(result, []) | ||
|
|
||
| def test_no_weekend_overlap(self): | ||
| holidays = [date(2025, 3, 4), date(2025, 3, 5), date(2025, 3, 6)] | ||
| instance = MockHolidayBase(holidays) | ||
| result = list_long_weekends(instance, require_weekend_overlap=False) | ||
| self.assertEqual(result, [[date(2025, 3, 4), date(2025, 3, 5), date(2025, 3, 6)]]) | ||
|
|
||
| def test_holidays_less_than_three_days(self): | ||
| holidays = [date(2025, 5, 3)] | ||
| instance = MockHolidayBase(holidays) | ||
| result = list_long_weekends(instance) | ||
| self.assertEqual(result, []) |
There was a problem hiding this comment.
I did change this to use a common helper method, merge some similar test cases and add some missing ones (the ones which has long weekends straddling across 2 years, as well as real country and financial holidays examples)
| class TestListLongWeekends(unittest.TestCase): | |
| def test_simple_long_weekend(self): | |
| holidays = [date(2025, 7, 4)] | |
| instance = MockHolidayBase(holidays) | |
| result = list_long_weekends(instance) | |
| self.assertEqual(result, [[date(2025, 7, 4), date(2025, 7, 5), date(2025, 7, 6)]]) | |
| def test_multiple_holidays(self): | |
| holidays = [date(2025, 7, 4), date(2025, 12, 26)] | |
| instance = MockHolidayBase(holidays) | |
| result = list_long_weekends(instance) | |
| expected = [ | |
| [date(2025, 7, 4), date(2025, 7, 5), date(2025, 7, 6)], | |
| [date(2025, 12, 26), date(2025, 12, 27), date(2025, 12, 28)], | |
| ] | |
| self.assertEqual(result, expected) | |
| def test_three_holidays_no_weekend(self): | |
| holidays = [date(2025, 3, 4), date(2025, 3, 5), date(2025, 3, 6)] | |
| instance = MockHolidayBase(holidays) | |
| result = list_long_weekends(instance) | |
| self.assertEqual(result, []) | |
| def test_three_holidays_included_with_flag(self): | |
| holidays = [date(2025, 3, 4), date(2025, 3, 5), date(2025, 3, 6)] | |
| instance = MockHolidayBase(holidays) | |
| result = list_long_weekends(instance, require_weekend_overlap=False) | |
| self.assertEqual(result, [[date(2025, 3, 4), date(2025, 3, 5), date(2025, 3, 6)]]) | |
| def test_custom_weekend(self): | |
| holidays = [date(2025, 4, 10)] | |
| instance = MockHolidayBase(holidays, weekend={4, 5}) | |
| result = list_long_weekends(instance) | |
| self.assertEqual(result, [[date(2025, 4, 10), date(2025, 4, 11), date(2025, 4, 12)]]) | |
| def test_no_holidays(self): | |
| instance = MockHolidayBase([]) | |
| result = list_long_weekends(instance) | |
| self.assertEqual(result, []) | |
| def test_no_weekend_overlap(self): | |
| holidays = [date(2025, 3, 4), date(2025, 3, 5), date(2025, 3, 6)] | |
| instance = MockHolidayBase(holidays) | |
| result = list_long_weekends(instance, require_weekend_overlap=False) | |
| self.assertEqual(result, [[date(2025, 3, 4), date(2025, 3, 5), date(2025, 3, 6)]]) | |
| def test_holidays_less_than_three_days(self): | |
| holidays = [date(2025, 5, 3)] | |
| instance = MockHolidayBase(holidays) | |
| result = list_long_weekends(instance) | |
| self.assertEqual(result, []) | |
| class TestListLongWeekends(unittest.TestCase): | |
| def assertLongWeekendsEqual( # noqa: N802 | |
| self, | |
| holidays, | |
| expected, | |
| weekend=None, | |
| minimum_holiday_length=3, | |
| *, | |
| require_weekend_overlap=True, | |
| ): | |
| instance = MockHolidayBase(holidays, weekend=weekend or {SAT, SUN}) | |
| result = list_long_weekends( | |
| instance, | |
| minimum_holiday_length=minimum_holiday_length, | |
| require_weekend_overlap=require_weekend_overlap, | |
| ) | |
| self.assertEqual(result, expected) | |
| def test_long_weekend_single(self): | |
| self.assertLongWeekendsEqual( | |
| [date(2025, 7, 4)], | |
| [[date(2025, 7, 4), date(2025, 7, 5), date(2025, 7, 6)]], | |
| ) | |
| def test_long_weekend_multiple(self): | |
| self.assertLongWeekendsEqual( | |
| [date(2025, 7, 4), date(2025, 12, 26)], | |
| [ | |
| [date(2025, 7, 4), date(2025, 7, 5), date(2025, 7, 6)], | |
| [date(2025, 12, 26), date(2025, 12, 27), date(2025, 12, 28)], | |
| ], | |
| ) | |
| def test_long_weekend_require_weekend_overlap(self): | |
| self.assertLongWeekendsEqual( | |
| [date(2025, 3, 4), date(2025, 3, 5), date(2025, 3, 6)], | |
| [], | |
| ) | |
| self.assertLongWeekendsEqual( | |
| [date(2025, 3, 4), date(2025, 3, 5), date(2025, 3, 6)], | |
| [[date(2025, 3, 4), date(2025, 3, 5), date(2025, 3, 6)]], | |
| require_weekend_overlap=False, | |
| ) | |
| def test_long_weekend_custom_weekend(self): | |
| self.assertLongWeekendsEqual( | |
| [date(2025, 4, 10)], | |
| [[date(2025, 4, 10), date(2025, 4, 11), date(2025, 4, 12)]], | |
| weekend={FRI, SAT}, | |
| ) | |
| def test_long_weekend_no_holidays(self): | |
| self.assertLongWeekendsEqual( | |
| [], | |
| [], | |
| ) | |
| def test_long_weekend_custom_minimum_length(self): | |
| self.assertLongWeekendsEqual( | |
| [date(2025, 8, 11), date(2025, 8, 12), date(2025, 12, 5)], | |
| [[date(2025, 8, 9), date(2025, 8, 10), date(2025, 8, 11), date(2025, 8, 12)]], | |
| minimum_holiday_length=4, | |
| ) | |
| def test_long_weekend_across_years(self): | |
| self.assertLongWeekendsEqual( | |
| [date(2024, 1, 1)], | |
| [[date(2023, 12, 30), date(2023, 12, 31), date(2024, 1, 1)]], | |
| ) | |
| self.assertLongWeekendsEqual( | |
| [date(2021, 12, 31)], | |
| [[date(2021, 12, 31), date(2022, 1, 1), date(2022, 1, 2)]], | |
| ) | |
| def test_long_weekend_real_country_holidays_data(self): | |
| self.assertEqual( | |
| list_long_weekends(country_holidays("AU", subdiv="NSW", years=2024)), | |
| [ | |
| [date(2023, 12, 30), date(2023, 12, 31), date(2024, 1, 1)], | |
| [date(2024, 1, 26), date(2024, 1, 27), date(2024, 1, 28)], | |
| [date(2024, 3, 29), date(2024, 3, 30), date(2024, 3, 31), date(2024, 4, 1)], | |
| [date(2024, 6, 8), date(2024, 6, 9), date(2024, 6, 10)], | |
| [date(2024, 10, 5), date(2024, 10, 6), date(2024, 10, 7)], | |
| ], | |
| ) | |
| def test_long_weekend_real_financial_holidays_data(self): | |
| self.assertEqual( | |
| list_long_weekends(financial_holidays("XNSE", years=2024)), | |
| [ | |
| [date(2024, 1, 26), date(2024, 1, 27), date(2024, 1, 28)], | |
| [date(2024, 3, 8), date(2024, 3, 9), date(2024, 3, 10)], | |
| [date(2024, 3, 23), date(2024, 3, 24), date(2024, 3, 25)], | |
| [date(2024, 3, 29), date(2024, 3, 30), date(2024, 3, 31)], | |
| [date(2024, 6, 15), date(2024, 6, 16), date(2024, 6, 17)], | |
| [date(2024, 11, 1), date(2024, 11, 2), date(2024, 11, 3)], | |
| [date(2024, 11, 15), date(2024, 11, 16), date(2024, 11, 17)], | |
| ], | |
| ) |
| def list_long_weekends( | ||
| instance: HolidayBase, | ||
| minimum_holiday_length: int = 3, | ||
| require_weekend_overlap: Optional[bool] = True, | ||
| ) -> list: |
There was a problem hiding this comment.
| def list_long_weekends( | |
| instance: HolidayBase, | |
| minimum_holiday_length: int = 3, | |
| require_weekend_overlap: Optional[bool] = True, | |
| ) -> list: | |
| def list_long_weekends( | |
| instance: HolidayBase, *, minimum_holiday_length: int = 3, require_weekend_overlap: bool = True | |
| ) -> list[list[date]]: |
To make boolean parameters keyword-only in new code.
Proposed change
Resolves #2422
Type of change
holidaysfunctionality in general)Checklist
make checklocally; all checks and tests passed.