Update `HolidayBase::get_closest_holiday` method by gaoflow · Pull Request #3651 · vacanza/holidays

gaoflow · 2026-06-25T07:31:04Z

What

get_closest_holiday looks into the adjacent year when the search would cross a year boundary. It called _populate(next_year) / _populate(previous_year) but never added the year to self.years. This left the object in an inconsistent state:

self.years reported only the originally loaded years even though the adjacent year's data was already in the dict.
Every subsequent call to get_closest_holiday for the same date range saw the adjacent year still missing from self.years, so it called _populate again unnecessarily.

Reproduction

import holidays
from datetime import date

us = holidays.US(years=2024, expand=False)
print(us.years)                                         # {2024}

us.get_closest_holiday(date(2024, 12, 31), "forward")
print(us.years)                                         # {2024}  ← BUG: 2025 missing
print(len(us))                                          # 22 (2025 data IS in the dict)

# Second call redundantly calls _populate(2025) again
us.get_closest_holiday(date(2024, 12, 31), "forward")
print(us.years)                                         # {2024}  ← still wrong

Fix

Add self.years.add(next_year) / self.years.add(previous_year) before the corresponding _populate calls, matching the pattern used in __keytransform__ (the normal expansion path).

Tests

Added test_get_closest_holiday_updates_years to TestClosestHoliday asserting that self.years is updated in both the forward and backward directions.

This pull request was prepared with the assistance of AI, under my direction and review.

When get_closest_holiday needed to look into an adjacent year it called _populate(next_year/previous_year) but never added that year to self.years. This left the object in an inconsistent state: the populated data was in the dict but years did not reflect it. As a result, every subsequent call to get_closest_holiday for the same date range would re-invoke _populate unnecessarily. Add the target year to self.years before calling _populate, matching the behaviour of __keytransform__ (the normal expansion path).

coderabbitai · 2026-06-25T07:31:23Z

Walkthrough

get_closest_holiday now records adjacent years in self.years when expansion is enabled, while tests verify forward, backward, and expand=False behavior.

Changes

Closest holiday year cache update

Layer / File(s)	Summary
Update cached years in closest-holiday search `holidays/holiday_base.py`, `tests/test_holiday_base.py`	`get_closest_holiday` adds the next or previous year to `self.years` before populating it when expansion is enabled; tests verify state changes and preservation when expansion is disabled.

Estimated code review effort: 2 (Simple) | ~10 minutes

Possibly related PRs

vacanza/holidays#2517: Updates closest-holiday adjacent-year behavior and related tests.

Suggested reviewers: ppsyrius, arkid15r, kjhellico

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 25.00% which is insufficient. The required threshold is 80.00%.	Write docstrings for the functions missing them to satisfy the coverage threshold.

✅ Passed checks (4 passed)

Check name	Status	Explanation
Linked Issues check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Title check	✅ Passed	The title is concise and directly describes the method being updated.
Description check	✅ Passed	The description matches the code changes and explains the state-tracking fix and tests.

✨ Finishing Touches

🧪 Generate unit tests (beta)

Create PR with unit tests

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.}

Copilot

Copilot encountered an error and was unable to review this pull request. You can try again by re-requesting a review.

cubic-dev-ai

No issues found across 2 files

Confidence score: 5/5

Automated review surfaced no issues in the provided summaries.
No files require special attention.

_{Re-trigger cubic}

codecov · 2026-06-25T14:55:40Z

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 100.00%. Comparing base (d6af602) to head (a37d591).

Additional details and impacted files

@@            Coverage Diff            @@
##               dev     #3651   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files          317       317           
  Lines        19007     19009    +2     
  Branches      2430      2430           
=========================================
+ Hits         19007     19009    +2

☔ View full report in Codecov by Harness.
📢 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.

KJhellico · 2026-06-25T15:36:32Z

        if direction == "forward" and (next_year := dt.year + 1) not in self.years:
+            self.years.add(next_year)
            self._populate(next_year)
        elif direction == "backward" and (previous_year := dt.year - 1) not in self.years:
+            self.years.add(previous_year)
            self._populate(previous_year)


I think, in general, it would be more accurate here to consider the value of self.expand rather than simply adding an extra year.

eeshsaxena

Nice catch - this is a real inconsistency. I pulled the branch and confirmed the behavior on dev before the fix:

US(years=2024, expand=False) then get_closest_holiday(date(2024, 12, 31), "forward") leaves self.years == {2024} even though 2025's holidays are already in the dict (any(d.year == 2025 for d in us.keys()) is True). So the object really is in a state where the data and self.years disagree. Same thing in the "backward" direction with 2023.

A couple of things I checked that might be useful for the review:

Completeness: these two call sites look like the only _populate() calls that load a year which isn't already in self.years without recording it. The other callers (__init__, the re-populate loop after clear()) iterate over self.years, and __keytransform__ already does self.years.add(...) right before _populate(...). So the fix covers both affected branches.
Ordering: adding the year before calling _populate() is safe here - _populate() doesn't consult self.years, and this matches the existing __keytransform__ pattern (self.years.add(dt.year); self._populate(dt.year)).
One thing I was initially worried about but which turned out fine: without the fix, a repeated get_closest_holiday call re-runs _populate() for the adjacent year (since it's still missing from self.years), and _populate documents that it "assumes that no holidays for the year have already been populated". In practice __setitem__ dedupes identical names, so it doesn't corrupt the data - but the fix does avoid that redundant re-population, which is a nice side benefit worth keeping in the description.

One small, optional thought (very much defer to the maintainers): the self.years.add(year); self._populate(year) pair now appears in three places, and this bug came from a _populate() call that was missing its paired add. A tiny helper that does both could keep them in sync and make future call sites harder to get wrong - but that's a maintainability nicety beyond the scope of this fix.

Test looks good and exercises both directions. Thanks for tracking this down!

eeshsaxena

Nice catch - this looks correct to me, and it lines up with how the rest of the class manages self.years.

The convincing bit is that the fix mirrors the canonical expand path in __keytransform__:

if self.expand and dt.year not in self.years:
    self.years.add(dt.year)
    self._populate(dt.year)

i.e. the established contract is that the caller adds the year to self.years before calling _populate (_populate itself only generates holidays and never touches self.years). get_closest_holiday was the one place calling _populate without first doing the self.years.add(...), so the object was left inconsistent exactly as described. Adding the year first - in the same order as __keytransform__ - is the right fix.

I applied the patch on top of dev and ran the get_closest_holiday tests: all pass, including the new test_get_closest_holiday_updates_years covering both the forward (+1) and backward (-1) branches. Good that both directions are exercised.

One tiny thing you might optionally note (not a blocker, and it matches existing behaviour): if the adjacent year falls outside [start_year, end_year], _populate early-returns, so the year gets added to self.years with no data - but that's already exactly what the __keytransform__ expand path does too, so this stays consistent with the current contract.

LGTM from me - deferring to the maintainers for the merge decision.

gaoflow · 2026-07-09T08:34:00Z

Thanks for verifying both directions. On the optional point: the out-of-[start_year, end_year] case is intentional. Adding the year before _populate early-returns leaves self.years holding a data-less year, but that's exactly the existing __keytransform__ expand contract — so I kept it consistent rather than special-casing get_closest_holiday. Happy to add a range guard if the maintainers would prefer, though that would diverge from __keytransform__.

sanmaxdev

Verified the fix locally. All 7 TestClosestHoliday tests pass, including the new regression test.

The inconsistency is real: before this PR, _populate(next_year) was called but the year was never added to self.years, so repeated calls to get_closest_holiday would re-populate the same year every time.

Regarding the expand point: the canonical pattern in __keytransform__ (line 650) gates both self.years.add() and self._populate() on self.expand. However, get_closest_holiday already calls _populate() unconditionally (the pre-existing code did not check self.expand). So the method intentionally crosses year boundaries regardless of expand.

If the maintainer prefers keeping self.years fixed when expand=False, a middle ground would be to guard only the self.years.add() calls on self.expand, accepting redundant _populate() calls for the expand=False case. If get_closest_holiday is expected to work across year boundaries regardless of expand (as the pre-existing code suggests), the current approach of always tracking populated years is the right one.

KJhellico · 2026-07-14T16:46:39Z

If the maintainer prefers keeping self.years fixed when expand=False

Yes, I think we missed this point when adding this method, and we need to fix it. If someone sets expand = False, he obviously expects that HolidayBase object will not change its state while being accessed.

…pand=True

gaoflow · 2026-07-15T07:50:44Z

Agreed — pushed 33d2910. The adjacent-year population is now gated on self.expand, mirroring __keytransform__, so with expand=False the object state (years and keys) stays untouched and the search only covers the populated years. Added a test asserting both the unchanged state and the backward result within the populated range.

sonarqubecloud · 2026-07-15T07:52:17Z

Quality Gate passed

Issues
0 New issues
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

KJhellico · 2026-07-15T13:25:39Z

-            self._populate(next_year)
-        elif direction == "backward" and (previous_year := dt.year - 1) not in self.years:
-            self._populate(previous_year)
+        # Automatically expand for `expand=True` cases only, like `__keytransform__`.


Suggested change

# Automatically expand for `expand=True` cases only, like `__keytransform__`.

KJhellico · 2026-07-15T13:25:49Z

+        # Searching forward should add the next year to self.years so that
+        # repeated calls do not re-populate the adjacent year.


Suggested change

# Searching forward should add the next year to self.years so that

# repeated calls do not re-populate the adjacent year.

KJhellico · 2026-07-15T13:26:19Z

+        # Searching backward should add the previous year to self.years.
+        us2 = US(years=2024)
+        self.assertEqual(us2.years, {2024})
+
+        us2.get_closest_holiday(date(2024, 1, 1), "backward")
+        self.assertIn(2023, us2.years)


Suggested change

# Searching backward should add the previous year to self.years.

us2 = US(years=2024)

self.assertEqual(us2.years, {2024})

us2.get_closest_holiday(date(2024, 1, 1), "backward")

self.assertIn(2023, us2.years)

us_2 = US(years=2024)

self.assertEqual(us_2.years, {2024})

us_2.get_closest_holiday(date(2024, 1, 1), "backward")

self.assertIn(2023, us_2.years)

KJhellico · 2026-07-15T13:26:35Z

+        self.assertIn(2023, us2.years)
+
+    def test_get_closest_holiday_expand_false_keeps_state(self):
+        # With expand=False the object must not change state while accessed.


Suggested change

# With expand=False the object must not change state while accessed.

gaoflow requested review from KJhellico, PPsyrius and arkid15r as code owners June 25, 2026 07:31

Copilot AI review requested due to automatic review settings June 25, 2026 07:31

github-actions Bot added the test label Jun 25, 2026

Copilot started reviewing on behalf of gaoflow June 25, 2026 07:31 View session

Copilot AI reviewed Jun 25, 2026

cubic-dev-ai Bot reviewed Jun 25, 2026

View reviewed changes

KJhellico reviewed Jun 25, 2026

View reviewed changes

eeshsaxena reviewed Jul 8, 2026

View reviewed changes

eeshsaxena reviewed Jul 9, 2026

View reviewed changes

sanmaxdev reviewed Jul 14, 2026

View reviewed changes

fix: only auto-populate adjacent years in get_closest_holiday when ex…

33d2910

…pand=True

KJhellico reviewed Jul 15, 2026

View reviewed changes

KJhellico changed the title ~~fix: add populated year to years set in get_closest_holiday~~ Update HolidayBase::get_closest_holiday method Jul 15, 2026

		# Searching forward should add the next year to self.years so that
		# repeated calls do not re-populate the adjacent year.

Uh oh!

Uh oh!

Conversation

gaoflow commented Jun 25, 2026

What

Reproduction

Fix

Tests

Uh oh!

coderabbitai Bot commented Jun 25, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Walkthrough

Changes

❌ Failed checks (1 warning)

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Uh oh!

cubic-dev-ai Bot left a comment

Choose a reason for hiding this comment

Uh oh!

codecov Bot commented Jun 25, 2026

Codecov Report

Uh oh!

KJhellico Jun 25, 2026

Choose a reason for hiding this comment

Uh oh!

eeshsaxena left a comment

Choose a reason for hiding this comment

Uh oh!

eeshsaxena left a comment

Choose a reason for hiding this comment

Uh oh!

gaoflow commented Jul 9, 2026

Uh oh!

sanmaxdev left a comment

Choose a reason for hiding this comment

Uh oh!

KJhellico commented Jul 14, 2026

Uh oh!

gaoflow commented Jul 15, 2026

Uh oh!

sonarqubecloud Bot commented Jul 15, 2026

Quality Gate passed

Uh oh!

KJhellico Jul 15, 2026

Choose a reason for hiding this comment

Uh oh!

KJhellico Jul 15, 2026

Choose a reason for hiding this comment

Uh oh!

KJhellico Jul 15, 2026

Choose a reason for hiding this comment

Uh oh!

KJhellico Jul 15, 2026

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

coderabbitai Bot commented Jun 25, 2026 •

edited

Loading