+
Skip to content

Conversation

PPsyrius
Copy link
Collaborator

Proposed change

Reduce l10n string duplication in Cuba's holiday code.

This is part of the non-test case changes decoupling from #2881

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

Copy link
Contributor

coderabbitai bot commented Sep 16, 2025

Summary by CodeRabbit

  • Refactor
    • Standardized the naming of Cuba’s July 25 and July 27 holidays by using a shared translation, ensuring consistent labels across both dates.
    • No changes to holiday dates, behavior, or public interfaces; end users and integrations will see the same results as before.
    • This update reduces duplication and improves maintainability without affecting outputs, configurations, or compatibility.

Walkthrough

Refactors Cuba holiday definitions by introducing a shared translated name for the Moncada commemoration and passing it to both Jul 25 and Jul 27 holiday additions, replacing direct translation calls. No public API changes.

Changes

Cohort / File(s) Change summary
Cuba holidays refactor
holidays/countries/cuba.py
Consolidates the Moncada commemoration label via a local variable (tr(...)) and uses it for both _add_holiday_jul_25(name) and _add_holiday_jul_27(name); removes separate direct translation call for Jul 27.

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~10 minutes

Suggested labels

l10n

Suggested reviewers

  • arkid15r
  • KJhellico

Pre-merge checks and finishing touches

❌ Failed checks (1 warning)
Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. You can run @coderabbitai generate docstrings to improve docstring coverage.
✅ Passed checks (2 passed)
Check name Status Explanation
Title Check ✅ Passed The title "Refactor Cuba holidays: reduce l10n string duplication" succinctly and accurately summarizes the main change (consolidating duplicate localization strings in Cuba's holiday code) and is clear and specific enough for a quick scan by teammates.
Description Check ✅ Passed The PR description directly states the intent to reduce l10n duplication in Cuba's holiday code, references related PR context and local checks, and matches the changes shown in the diff, so it is related and sufficient for this lenient check.
✨ 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 7347cd0 and 7a08482.

📒 Files selected for processing (1)
  • holidays/countries/cuba.py (1 hunks)
🧰 Additional context used
🧠 Learnings (4)
📓 Common learnings
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.
Learnt from: KJhellico
PR: vacanza/holidays#2608
File: tests/countries/test_saint_vincent_and_the_grenadines.py:162-178
Timestamp: 2025-07-02T18:21:59.302Z
Learning: In the Saint Vincent and the Grenadines holiday tests, for holidays without observed rules that only require a single assertHolidayName call, pass the holiday name directly as a string literal rather than storing it in a variable first for cleaner, more concise code.
📚 Learning: 2025-08-21T04:56:03.780Z
Learnt from: PPsyrius
PR: vacanza/holidays#2843
File: holidays/countries/burundi.py:63-101
Timestamp: 2025-08-21T04:56:03.780Z
Learning: In the holidays library, countries with localization support consistently use tr() wrappers around holiday names when calling _add_* methods (e.g., self._add_new_years_day(tr("Holiday Name"))). This is the established pattern across United States, Thailand, and other l10n-enabled countries, contrary to any suggestion that translation is handled internally by _add_* methods.

Applied to files:

  • holidays/countries/cuba.py
📚 Learning: 2025-06-16T14:08:09.492Z
Learnt from: KJhellico
PR: vacanza/holidays#2635
File: holidays/countries/bhutan.py:13-16
Timestamp: 2025-06-16T14:08:09.492Z
Learning: In the holidays library, translation is performed in the `_add_holiday` methods, not at the string level. Holiday strings passed to `_add_holiday_*` methods are processed through the translation machinery within those methods, so using `tr()` or `self.tr()` wrappers is unnecessary.

Applied to files:

  • holidays/countries/cuba.py
📚 Learning: 2025-08-21T04:56:03.780Z
Learnt from: PPsyrius
PR: vacanza/holidays#2843
File: holidays/countries/burundi.py:63-101
Timestamp: 2025-08-21T04:56:03.780Z
Learning: In the holidays library, countries with localization support DO use tr() wrappers around holiday names when calling _add_* methods. This is the correct pattern for l10n-enabled country implementations, contrary to previous learning about translation being handled internally by _add_* methods.

Applied to files:

  • holidays/countries/cuba.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). (2)
  • GitHub Check: Build distribution
  • GitHub Check: Test docs build
🔇 Additional comments (1)
holidays/countries/cuba.py (1)

83-85: Deduplicated Moncada label looks good.

Keeps the same msgid, matches this file’s tr() usage pattern, and preserves behavior for both Jul 25 and Jul 27. LGTM.

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.12.2)
holidays/countries/cuba.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.

Copy link

Copy link

codecov bot commented Sep 16, 2025

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 100.00%. Comparing base (7347cd0) to head (7a08482).
⚠️ Report is 4 commits behind head on dev.

Additional details and impacted files
@@            Coverage Diff            @@
##               dev     #2935   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files          296       296           
  Lines        17593     17594    +1     
  Branches      2270      2270           
=========================================
+ Hits         17593     17594    +1     

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

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!

@arkid15r arkid15r added this pull request to the merge queue Sep 16, 2025
Copy link
Collaborator

@arkid15r arkid15r left a comment

Choose a reason for hiding this comment

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

LGTM

Merged via the queue into vacanza:dev with commit 6f3e45f Sep 16, 2025
36 checks passed
@PPsyrius PPsyrius deleted the cuba_refactor branch September 17, 2025 02:58
@arkid15r arkid15r mentioned this pull request Oct 6, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants

点击 这是indexloc提供的php浏览器服务,不要输入任何密码和下载