Long weekend Function added #3001
Conversation
Summary by CodeRabbit
WalkthroughAdds a new public utility function list_long_weekends(country, year, subdiv=None) in holidays/utils.py to compute long holiday weekends, exports it via all, and updates tests to cover the new API and verify financial markets parity with source files. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Suggested reviewers
Pre-merge checks and finishing touches❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
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_*).
| def list_long_weekends( | ||
| country: str, | ||
| year: int, | ||
| subdiv: Optional[str] = None, | ||
| ) -> list: | ||
| try: | ||
| holidays_obj = country_holidays(country, subdiv=subdiv, years=year) | ||
| except NotImplementedError: | ||
| return [] | ||
|
|
||
| checked = set() | ||
| long_weekends = [] | ||
|
|
||
| holidays_in_year = set(holidays_obj.keys()) | ||
|
|
||
| for hol in sorted(holidays_in_year): | ||
| if hol in checked: | ||
| continue | ||
|
|
||
| prev_work = holidays_obj.get_nth_working_day(hol, -1) | ||
| next_work = holidays_obj.get_nth_working_day(hol, +1) | ||
|
|
||
| start = prev_work + timedelta(days=1) | ||
| end = next_work - timedelta(days=1) | ||
|
|
||
| length = (end - start).days + 1 | ||
|
|
||
| has_weekend = any( | ||
| (d.weekday() in getattr(holidays_obj, "weekend", {5, 6})) | ||
| for d in (start + timedelta(days=i) for i in range(length)) | ||
| ) | ||
|
|
||
| if length >= 3 and has_weekend: | ||
| long_weekends.append( | ||
| { | ||
| "start": start, | ||
| "end": end, | ||
| "length": length, | ||
| } | ||
| ) | ||
|
|
||
| for d in holidays_in_year: | ||
| if start <= d <= end: | ||
| checked.add(d) | ||
|
|
||
| return long_weekends |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Solid algorithm; add docstring, precise typing, and small perf tidy
- Add a docstring to document behavior (incl. cross-year spans) and return shape.
- Tighten the return type to list[dict[str, Any]] (or a NamedTuple/dataclass) for public API clarity.
- Cache weekend set once to avoid repeated getattr in the inner loop.
- def list_long_weekends(
+ def list_long_weekends(
country: str,
year: int,
subdiv: Optional[str] = None,
-) -> list:
+) -> list[dict[str, Any]]:
+ """
+ Compute holiday-based long weekends for a given country/year.
+
+ A long weekend is the maximal consecutive non-working block around each
+ holiday (using is_working_day and entity-specific weekend), with:
+ - start: the day after the previous working day
+ - end: the day before the next working day
+ - length: inclusive day count
+ Only blocks with length >= 3 that include at least one weekend day are returned.
+ Note: start/end can span outside the given year (e.g., Jan 1 on Monday).
+ Returns [] for unknown country/subdivision.
+ """
try:
holidays_obj = country_holidays(country, subdiv=subdiv, years=year)
except NotImplementedError:
return []
checked = set()
long_weekends = []
holidays_in_year = set(holidays_obj.keys())
+ weekend = getattr(holidays_obj, "weekend", {5, 6})
for hol in sorted(holidays_in_year):
if hol in checked:
continue
prev_work = holidays_obj.get_nth_working_day(hol, -1)
next_work = holidays_obj.get_nth_working_day(hol, +1)
start = prev_work + timedelta(days=1)
end = next_work - timedelta(days=1)
length = (end - start).days + 1
- has_weekend = any(
- (d.weekday() in getattr(holidays_obj, "weekend", {5, 6}))
- for d in (start + timedelta(days=i) for i in range(length))
- )
+ has_weekend = any((start + timedelta(days=i)).weekday() in weekend for i in range(length))And update typing imports:
-from typing import Optional, Union
+from typing import Any, Optional, UnionQuestion: do we want to also include an optional parameter to suggest “potential” long weekends (Thu/Tue) as the linked issue describes? If yes, I can propose a minimal, backward-compatible flag and tests.
📝 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.
| def list_long_weekends( | |
| country: str, | |
| year: int, | |
| subdiv: Optional[str] = None, | |
| ) -> list: | |
| try: | |
| holidays_obj = country_holidays(country, subdiv=subdiv, years=year) | |
| except NotImplementedError: | |
| return [] | |
| checked = set() | |
| long_weekends = [] | |
| holidays_in_year = set(holidays_obj.keys()) | |
| for hol in sorted(holidays_in_year): | |
| if hol in checked: | |
| continue | |
| prev_work = holidays_obj.get_nth_working_day(hol, -1) | |
| next_work = holidays_obj.get_nth_working_day(hol, +1) | |
| start = prev_work + timedelta(days=1) | |
| end = next_work - timedelta(days=1) | |
| length = (end - start).days + 1 | |
| has_weekend = any( | |
| (d.weekday() in getattr(holidays_obj, "weekend", {5, 6})) | |
| for d in (start + timedelta(days=i) for i in range(length)) | |
| ) | |
| if length >= 3 and has_weekend: | |
| long_weekends.append( | |
| { | |
| "start": start, | |
| "end": end, | |
| "length": length, | |
| } | |
| ) | |
| for d in holidays_in_year: | |
| if start <= d <= end: | |
| checked.add(d) | |
| return long_weekends | |
| from typing import Any, Optional, Union | |
| def list_long_weekends( | |
| country: str, | |
| year: int, | |
| subdiv: Optional[str] = None, | |
| ) -> list[dict[str, Any]]: | |
| """ | |
| Compute holiday-based long weekends for a given country/year. | |
| A long weekend is the maximal consecutive non-working block around each | |
| holiday (using is_working_day and entity-specific weekend), with: | |
| - start: the day after the previous working day | |
| - end: the day before the next working day | |
| - length: inclusive day count | |
| Only blocks with length >= 3 that include at least one weekend day are returned. | |
| Note: start/end can span outside the given year (e.g., Jan 1 on Monday). | |
| Returns [] for unknown country/subdivision. | |
| """ | |
| try: | |
| holidays_obj = country_holidays(country, subdiv=subdiv, years=year) | |
| except NotImplementedError: | |
| return [] | |
| checked = set() | |
| long_weekends = [] | |
| holidays_in_year = set(holidays_obj.keys()) | |
| weekend = getattr(holidays_obj, "weekend", {5, 6}) | |
| for hol in sorted(holidays_in_year): | |
| if hol in checked: | |
| continue | |
| prev_work = holidays_obj.get_nth_working_day(hol, -1) | |
| next_work = holidays_obj.get_nth_working_day(hol, +1) | |
| start = prev_work + timedelta(days=1) | |
| end = next_work - timedelta(days=1) | |
| length = (end - start).days + 1 | |
| has_weekend = any( | |
| (start + timedelta(days=i)).weekday() in weekend | |
| for i in range(length) | |
| ) | |
| if length >= 3 and has_weekend: | |
| long_weekends.append( | |
| { | |
| "start": start, | |
| "end": end, | |
| "length": length, | |
| } | |
| ) | |
| for d in holidays_in_year: | |
| if start <= d <= end: | |
| checked.add(d) | |
| return long_weekends |
| 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.
| class TestListLongWeekends(unittest.TestCase): | ||
| def test_valid_country_usa(self): | ||
| results = list_long_weekends("US", 2024) | ||
| self.assertIsInstance(results, list) | ||
| self.assertTrue(any(r["length"] >= 3 for r in results)) | ||
|
|
||
| def test_invalid_country_code(self): | ||
| results = list_long_weekends("JA", 2024) | ||
| self.assertEqual(results, [], "Expected empty list for invalid country code") | ||
|
|
||
| def test_subdivision(self): | ||
| results = list_long_weekends("US", 2024, subdiv="CA") | ||
| self.assertIsInstance(results, list) | ||
|
|
||
| def test_long_weekend_structure(self): | ||
| results = list_long_weekends("IN", 2024) | ||
| for w in results: | ||
| self.assertIsInstance(w["start"], date) | ||
| self.assertIsInstance(w["end"], date) | ||
| self.assertIsInstance(w["length"], int) | ||
|
|
||
| def test_empty_year(self): | ||
| results = list_long_weekends("US", 2500) | ||
| self.assertIsInstance(results, list) |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Strengthen long-weekends tests and clarify expectations
- Consider asserting that all returned blocks have length >= 3 (the function enforces this), and add a cross-year case (e.g., US 2024 New Year’s spans 2023-12-30 to 2024-01-01).
- Optional: add a test for invalid subdivision to mirror the invalid country case.
- PT009 from Ruff suggests bare asserts, but this project consistently uses unittest assertions inside TestCase. Safe to ignore PT009 here. Based on learnings
Would you like to add a boundary test like this?
def test_long_weekend_cross_year_boundary(self):
# Jan 1, 2024 (US) yields a long weekend starting in the previous year.
results = list_long_weekends("US", 2024)
self.assertTrue(any(w["start"].year == 2023 and w["end"].year == 2024 for w in results))
def test_invalid_subdivision(self):
self.assertEqual(list_long_weekends("US", 2024, subdiv="ZZ"), [])🧰 Tools
🪛 Ruff (0.13.3)
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)
🤖 Prompt for AI Agents
In tests/test_utils.py around lines 229 to 252, strengthen the long-weekends
tests by (1) asserting that every returned block has length >= 3 using unittest
assertions (e.g., self.assertTrue(all(w["length"] >= 3 for w in results))), (2)
add a cross-year boundary test that checks at least one returned weekend for US
2024 spans 2023 to 2024 (e.g., self.assertTrue(any(w["start"].year == 2023 and
w["end"].year == 2024 for w in results))), and (3) add an invalid subdivision
test mirroring the invalid country case to assert list_long_weekends("US", 2024,
subdiv="ZZ") returns []. Use TestCase assertion methods (no bare asserts) and
keep tests concise.
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 305 305
Lines 17998 18022 +24
Branches 2323 2328 +5
=========================================
+ Hits 17998 18022 +24 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
| country: str, | ||
| year: int, | ||
| subdiv: Optional[str] = None, |
There was a problem hiding this comment.
I think it would be better to have an instance: HolidayBase parameter (as in ICalExporter) instead of separate country and subdiv (and possible observed etc.)
Also, the result could be something like list[list[date]] instead of list of dicts.
| (d.weekday() in getattr(holidays_obj, "weekend", {5, 6})) | ||
| for d in (start + timedelta(days=i) for i in range(length)) | ||
| ) | ||
|
|
There was a problem hiding this comment.
What if there are 3+ holidays in a row, without a weekend?
There was a problem hiding this comment.
If there are 3+ holidays without weekend then it would not be included in the function.
As much as I know long weekend means weekend plus 1 or more holidays in the preceding or following days.
Proposed change
Resolves #2422
Type of change
holidaysfunctionality in general)Checklist
make checklocally; all checks and tests passed.