Repository navigation
fix(common): import holidays lazily so a broken install can't stop the integration - #2345
Merged
Merged
Conversation
…e integration Only one scraper (Vale of White Horse) uses the `holidays` package, but common.py imported it at module level and every scraper imports common. A broken `holidays` install, as in #2335 ("No module named 'holidays.calendars.hebrew'" after a Home Assistant core update, which is the signature of a half-upgraded package), therefore stopped the whole integration from loading for all ~350 councils. Import it inside is_holiday() instead, so such a failure now only affects that one council. A subprocess test puts a deliberately broken `holidays` package on the path and checks that `common` and `collect_data` still import; it fails on the old code with the same traceback as the report. Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_015MaCS29izthZp2dwtvLwf4
Contributor
|
Warning Review limit reachedYou've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. Next included review available in 43 minutes. View limit details
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. |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #2345 +/- ##
=======================================
Coverage 83.57% 83.57%
=======================================
Files 43 43
Lines 3380 3380
=======================================
Hits 2825 2825
Misses 555 555 ☔ View full report in Codecov by Harness. |
2 tasks done
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Why
#2335: after a Home Assistant core update the integration fails to load at all:
What's actually going on
holidays.calendars.hebrewexists in theholidaysversion this project locks (0.103), and our code only does a plainimport holidays. A missing submodule inside a package that is otherwise present is the signature of a half-upgradedholidaysinstall in the user's Home Assistant container (HA core and HACS both install it). So the root cause is in their environment, and I can't fix it from here.What I can fix is the blast radius. Only one scraper (Vale of White Horse) uses
holidays, yetcommon.pyimported it at module level and every scraper importscommon. One bad package therefore took down all ~350 councils, including the ones that never touch it.Change
Import
holidaysinsideis_holiday()instead. Now a broken install only affects that one council, and everyone else keeps working.Existing
is_holidaytests patchholidays.country_holidayson the module object, so they're unaffected (they still pass).Test
test_holidays_import_is_lazy.pyruns in a subprocess (so the deliberately broken package can't leak into other tests) with a fakeholidayspackage on the path that raises the exact error from the report. It asserts thatcommonandcollect_datastill import, and that onlyis_holiday()fails.I confirmed it reproduces the problem: against master's
common.pyit fails withModuleNotFoundError: No module named 'holidays.calendars.hebrew'atcommon.py:10, the same traceback as the report. With the change it passes.Test plan
make unit-testson Python 3.14.10: 496 passed. The 7 failures are the same as master's (3 sandbox-only network tests, plus 4 already fixed by test: stop the Orkney and Broxbourne tests failing on the calendar and the network #2341).common.pyand passes with this change.lint.yml.common.py, so CI runs the full live suite.I used
Targetsrather thanFixesbecause the user's own install still needs repairing (restart / rebuild the container, orpip install --force-reinstall holidaysinside it). This change just stops one bad package taking everything else down.Targets #2335
Generated by Claude Code