Skip to content

Replace lru_cache with cache #2964

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Merged
merged 3 commits into from
Sep 27, 2025
Merged

Replace lru_cache with cache #2964

merged 3 commits into from
Sep 27, 2025

Conversation

@arkid15r
Copy link
Collaborator

@arkid15r arkid15r commented Sep 27, 2025

Proposed change

Migrate @lru_cache cases to use @cache. Based on #2951 (review)

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
Contributor

coderabbitai bot commented Sep 27, 2025 •
edited
Loading

Summary by CodeRabbit

  • Refactor
    • Upgraded internal caching to a modern mechanism across holiday calculations and listing utilities, improving performance and consistency without changing behavior.
    • Users may notice faster responses when working with Thai calendar dates, Asian lunar-based calculations, and when listing supported or localized countries and financial markets.

Walkthrough

Replaced functools.lru_cache with functools.cache in three modules; decorators updated on several functions/methods. No signature, control flow, or return-type changes.

Changes

Cohort / File(s) Summary
Thai lunisolar calendar
holidays/calendars/thai.py		 Import changed to from functools import cache; decorators on _get_start_date and buddhist_sabbath_dates updated from @lru_cache to @cache.
Utilities caching
holidays/utils.py		 Import changed to from functools import cache; decorators on list_localized_countries, list_localized_financial, list_supported_countries, and list_supported_financial updated from @lru_cache to @cache.
Asian calendar script
scripts/calendar/asian_generator.py		 Import changed to from functools import cache; decorators on _get_leap_month and _span_days updated from @lru_cache to @cache.

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~10 minutes

Possibly related PRs

Suggested reviewers

  • KJhellico

Pre-merge checks and finishing touches

✅ Passed checks (3 passed)
Check name Status Explanation
Title Check ✅ Passed The title “Replace lru_cache with cache” directly summarizes the primary change of migrating all @lru_cache usages to @cache, is concise and clear, and avoids unnecessary detail or noise, making it easy for teammates to understand the main intent at a glance.
Description Check ✅ Passed The pull request description clearly describes the core change—migrating @lru_cache to @cache—provides context with a reference to the related review discussion, and includes a checklist confirming guideline compliance and successful local tests, so it is relevant and on-topic.
Docstring Coverage ✅ Passed Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%.
✨ Finishing touches
  • 📝 Generate Docstrings
🧪 Generate unit tests
  • 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 a338074 and bdedc5e.

📒 Files selected for processing (1)
  • holidays/calendars/thai.py (3 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-09-21T05:16:38.578Z 
Learnt from: PPsyrius
PR: vacanza/holidays#2951
File: tests/calendars/test_thai.py:0-0
Timestamp: 2025-09-21T05:16:38.578Z
Learning: In holidays/calendars/thai.py, the `buddhist_sabbath_dates` method in `_ThaiLunisolar` is independent of calendar type (THAI_CALENDAR vs KHMER_CALENDAR) and calculates Buddhist Sabbath dates consistently regardless of which calendar is being used.

Applied to files:

  • holidays/calendars/thai.py
⏰ 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). (1)
  • GitHub Check: Test Python 3.13 on windows-latest
🔇 Additional comments (1)
holidays/calendars/thai.py (1)

298-326: Looks solid — happy with the cache migration.

The swap to functools.cache keeps the memoization we relied on while lifting the old 128-entry ceiling, which fits nicely for these year-based lookups. No follow-up needed.

Warning

There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure.

🔧 Ruff (0.13.1)
holidays/calendars/thai.py

�[1;31mruff failed�[0m
�[1mCause:�[0m Failed to load configuration /ruff.toml
�[1mCause:�[0m Failed to parse /ruff.toml
�[1mCause:�[0m TOML parse error at line 26, column 3
|
26 | "RSE100", # Use of assert detected
| ^^^^^^^^
Unknown rule selector: RSE100

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

codecov bot commented Sep 27, 2025 •
edited
Loading

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 100.00%. Comparing base (14c5c1e) to head (bdedc5e).
⚠️ Report is 1 commits behind head on dev.

Additional details and impacted files 
@@            Coverage Diff            @@
##               dev     #2964   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files          300       300           
  Lines        17955     17955           
  Branches      2326      2326           
=========================================
  Hits         17955     17955

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

@arkid15r arkid15r marked this pull request as ready for review September 27, 2025 18:07
@arkid15r arkid15r enabled auto-merge September 27, 2025 18:07
PPsyrius
PPsyrius previously approved these changes Sep 27, 2025
View reviewed changes
Copy link
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 added this pull request to the merge queue Sep 27, 2025
@KJhellico KJhellico removed this pull request from the merge queue due to a manual request Sep 27, 2025
@sonarqubecloud
Copy link

sonarqubecloud bot commented Sep 27, 2025

Quality Gate Passed 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 KJhellico enabled auto-merge September 27, 2025 18:29
KJhellico
KJhellico approved these changes Sep 27, 2025
View reviewed changes
Copy link
Collaborator

@KJhellico KJhellico left a comment

Choose a reason for hiding this comment

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

LGTM.

@KJhellico KJhellico added this pull request to the merge queue Sep 27, 2025
Merged via the queue into vacanza:dev with commit e7b54e5 Sep 27, 2025
36 checks passed
@arkid15r arkid15r mentioned this pull request Oct 6, 2025
Merged
@arkid15r arkid15r deleted the ark/lru-cache-migration branch October 6, 2025 23:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@KJhellico KJhellico KJhellico approved these changes

@PPsyrius PPsyrius PPsyrius left review comments

Assignees

No one assigned

Labels

script

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@arkid15r @KJhellico @PPsyrius