+
Skip to content

Conversation

PPsyrius
Copy link
Collaborator

Proposed change

Reduce l10n string duplication in Chile'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

  • New Features

    • None.
  • Bug Fixes

    • Improved accuracy of Chile’s Fiestas Patrias holidays: consistent naming across years and corrected observed-day rules from 2007 onward, including Friday-only observation for Sep 20 in 2008.
  • Refactor

    • Streamlined holiday naming to use a single translated label for Fiestas Patrias, reducing inconsistencies.
  • Chores

    • Minor clarifying comment added for New Year’s Day observation (no functional impact).

Walkthrough

Refactors Chile holidays logic to centralize the “Fiestas Patrias” translation into a variable, adjusts observed holiday handling for Sep 17 and Sep 20 within the 2007+ branch (including 2008 FRI_ONLY rule), adds a comment for New Year’s observed post-2017, and reorganizes related conditionals.

Changes

Cohort / File(s) Summary
Chile holidays refactor/observance
holidays/countries/chile.py
- Added comment above New Year’s Day observed (year ≥ 2017).
- Introduced name = tr("Fiestas Patrias") and reused across branches.
- Updated _add_holiday_sep_17/_20 calls to use shared name.
- In 2007+, applied observed to Sep 17; nested 2008 FRI_ONLY observed for Sep 20.
- Applied name in 1932–1944 Sep 20 branch.
- Moved 2008 Sep 20 observed inside 2007+ block; minor reorganization.

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 Chile holidays: reduce l10n string duplication" succinctly describes the primary change: reducing localization string duplication in the Chile holidays module. The raw_summary and PR objectives confirm the change centers on consolidating translated strings into a shared variable and reorganizing Fiestas Patrias handling. The title is concise, specific, and appropriate for a teammate scanning history.
Description Check ✅ Passed The pull request description clearly states the intent to reduce l10n string duplication in the Chile holidays code and notes its relation to a larger decoupling effort, which matches the raw_summary. It specifies the change type (refactor/cleanup), includes checklist information, and indicates tests/checks passed locally. Given the lenient pass criteria, the description is related to the changeset and sufficiently informative for reviewers.
✨ 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 89269c1.

📒 Files selected for processing (1)
  • holidays/countries/chile.py (2 hunks)
🧰 Additional context used
🧠 Learnings (14)
📓 Common learnings
Learnt from: mbfarah
PR: vacanza/holidays#2798
File: holidays/countries/chile.py:278-281
Timestamp: 2025-08-12T03:21:09.116Z
Learning: In Chile holidays implementation, the general rule for September 17 (years >= 2007) uses MON_ONLY for years 2007-2016, meaning it only adds the holiday when September 17 falls on a Monday. For years when September 17 falls on other days and was declared a one-off holiday by specific laws, it must be added to the special_public_holidays dictionary.
Learnt from: PPsyrius
PR: vacanza/holidays#2881
File: holidays/countries/chile.py:121-123
Timestamp: 2025-09-14T04:36:25.096Z
Learning: In the Chile holidays implementation, the MON_ONLY observance rule is used correctly for January 2 holiday. The rule means "only add this observed holiday if the original date falls on Monday", which perfectly handles Law 20.983's requirement that January 2 is only a holiday when January 1 falls on Sunday, since January 2 falls on Monday exactly when January 1 falls on Sunday.
Learnt from: mbfarah
PR: vacanza/holidays#2798
File: holidays/countries/chile.py:278-281
Timestamp: 2025-08-12T03:21:09.116Z
Learning: September 17, 2010 and September 20, 2010 were both one-off holidays in Chile established by Law 20.450, separate from the general September 17 observance rule that only applies to Mondays.
Learnt from: PPsyrius
PR: vacanza/holidays#2537
File: holidays/countries/finland.py:0-0
Timestamp: 2025-05-10T04:32:15.760Z
Learning: In the holidays package, detailed historical context and additional information should be added as comments at the method level or above conditional blocks, while comments directly above tr() function calls should only contain the holiday name itself (e.g., "# Independence Day.").
Learnt from: PPsyrius
PR: vacanza/holidays#2632
File: holidays/countries/solomon_islands.py:95-98
Timestamp: 2025-06-16T12:28:31.641Z
Learning: Library-wide holiday patterns and their optimizations should be handled at the base class level (like InternationalHolidays) rather than documenting workarounds in individual country modules. This maintains separation of concerns and avoids documentation duplication.
Learnt from: PPsyrius
PR: vacanza/holidays#2642
File: holidays/countries/france.py:300-319
Timestamp: 2025-06-18T10:21:01.376Z
Learning: In the France holidays implementation, legislative years for holiday changes should be hard-coded rather than extracted into constants, as this maintains consistency with the existing codebase pattern and provides historical accuracy for specific legislative acts.
Learnt from: PPsyrius
PR: vacanza/holidays#2881
File: holidays/countries/chile.py:121-123
Timestamp: 2025-09-14T04:36:25.096Z
Learning: In the Chile holidays implementation, the MON_ONLY observance rule correctly implements Law 20.983 for January 2. MON_ONLY is defined as ObservedRule({TUE: None, WED: None, THU: None, FRI: None, SAT: None, SUN: None}), which means when _add_observed() is called with MON_ONLY, it removes holidays that fall on Tuesday through Sunday (via self.pop(dt)) and only keeps holidays that fall on Monday. This perfectly implements the law that January 2 is only a holiday when January 1 falls on Sunday.
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#2834
File: tests/financial/test_national_stock_exchange_of_india.py:342-360
Timestamp: 2025-08-30T12:52:58.539Z
Learning: In the NSE holidays implementation, assertLocalizedHolidays should only include holidays that are actually observed (trading holidays), not holidays that fall on weekends and are excluded by the observed_rule. For example, Eid al-Fitr 2023 falls on Saturday and is correctly excluded from localization tests.
Learnt from: PPsyrius
PR: vacanza/holidays#2537
File: holidays/countries/finland.py:249-253
Timestamp: 2025-05-10T04:02:13.815Z
Learning: Holiday name comments directly above tr() function calls in the holidays package should only contain the holiday name itself (e.g., "# Independence Day.") without any additional context, dates, or historical information.
📚 Learning: 2025-08-12T03:21:09.116Z
Learnt from: mbfarah
PR: vacanza/holidays#2798
File: holidays/countries/chile.py:278-281
Timestamp: 2025-08-12T03:21:09.116Z
Learning: In Chile holidays implementation, the general rule for September 17 (years >= 2007) uses MON_ONLY for years 2007-2016, meaning it only adds the holiday when September 17 falls on a Monday. For years when September 17 falls on other days and was declared a one-off holiday by specific laws, it must be added to the special_public_holidays dictionary.

Applied to files:

  • holidays/countries/chile.py
📚 Learning: 2025-05-10T04:32:15.760Z
Learnt from: PPsyrius
PR: vacanza/holidays#2537
File: holidays/countries/finland.py:0-0
Timestamp: 2025-05-10T04:32:15.760Z
Learning: In the holidays package, detailed historical context and additional information should be added as comments at the method level or above conditional blocks, while comments directly above tr() function calls should only contain the holiday name itself (e.g., "# Independence Day.").

Applied to files:

  • holidays/countries/chile.py
📚 Learning: 2025-04-03T12:36:41.201Z
Learnt from: PPsyrius
PR: vacanza/holidays#2398
File: holidays/countries/guinea.py:73-77
Timestamp: 2025-04-03T12:36:41.201Z
Learning: In the Holidays library, comments explaining year restrictions for holidays should be placed above the year check conditional statement, not inside it. Example format:
```python
# reason why goes here
if start_year <= self._year <= end_year:
    # Holiday name
    self._add_holiday_function(tr("Holiday Name"))
```

Applied to files:

  • holidays/countries/chile.py
📚 Learning: 2025-05-10T04:02:13.815Z
Learnt from: PPsyrius
PR: vacanza/holidays#2537
File: holidays/countries/finland.py:249-253
Timestamp: 2025-05-10T04:02:13.815Z
Learning: Holiday name comments directly above tr() function calls in the holidays package should only contain the holiday name itself (e.g., "# Independence Day.") without any additional context, dates, or historical information.

Applied to files:

  • holidays/countries/chile.py
📚 Learning: 2025-08-25T09:57:22.291Z
Learnt from: PPsyrius
PR: vacanza/holidays#2833
File: holidays/countries/uganda.py:50-56
Timestamp: 2025-08-25T09:57:22.291Z
Learning: In the holidays package, when adding comments for year-restricted holidays in Uganda and similar country modules, include specific legal or historical context such as the actual laws, parliamentary acts, or government decisions that established the holidays, rather than just generic "since YEAR" information. For example, reference the specific Parliament act, decree, or historical event that created the holiday.

Applied to files:

  • holidays/countries/chile.py
📚 Learning: 2025-04-23T09:22:41.753Z
Learnt from: PPsyrius
PR: vacanza/holidays#2489
File: holidays/countries/sao_tome_and_principe.py:86-88
Timestamp: 2025-04-23T09:22:41.753Z
Learning: For holiday definitions in the holidays package, keep comments simple with just the holiday name (e.g., "# Independence Day.") rather than including dates or historical context, as the function names already encode the date information.

Applied to files:

  • holidays/countries/chile.py
📚 Learning: 2025-06-19T05:54:49.792Z
Learnt from: PPsyrius
PR: vacanza/holidays#2642
File: holidays/countries/french_polynesia.py:1-12
Timestamp: 2025-06-19T05:54:49.792Z
Learning: The holidays library uses a standard file header format across all country implementation files consisting of a comprehensive comment block with project description, authors, website, and license information. Country files do not use module-level docstrings but instead rely on this header format followed by class-level docstrings.

Applied to files:

  • holidays/countries/chile.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 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/chile.py
📚 Learning: 2025-09-14T04:36:25.096Z
Learnt from: PPsyrius
PR: vacanza/holidays#2881
File: holidays/countries/chile.py:121-123
Timestamp: 2025-09-14T04:36:25.096Z
Learning: In the Chile holidays implementation, the MON_ONLY observance rule is used correctly for January 2 holiday. The rule means "only add this observed holiday if the original date falls on Monday", which perfectly handles Law 20.983's requirement that January 2 is only a holiday when January 1 falls on Sunday, since January 2 falls on Monday exactly when January 1 falls on Sunday.

Applied to files:

  • holidays/countries/chile.py
📚 Learning: 2025-06-21T16:30:12.749Z
Learnt from: KJhellico
PR: vacanza/holidays#2654
File: holidays/countries/cape_verde.py:1-12
Timestamp: 2025-06-21T16:30:12.749Z
Learning: The holidays project does not use module docstrings in country holiday files. All country files start directly with the copyright header comment block without module docstrings, maintaining a consistent coding style across the project.

Applied to files:

  • holidays/countries/chile.py
📚 Learning: 2025-09-14T04:36:25.096Z
Learnt from: PPsyrius
PR: vacanza/holidays#2881
File: holidays/countries/chile.py:121-123
Timestamp: 2025-09-14T04:36:25.096Z
Learning: In the Chile holidays implementation, the MON_ONLY observance rule correctly implements Law 20.983 for January 2. MON_ONLY is defined as ObservedRule({TUE: None, WED: None, THU: None, FRI: None, SAT: None, SUN: None}), which means when _add_observed() is called with MON_ONLY, it removes holidays that fall on Tuesday through Sunday (via self.pop(dt)) and only keeps holidays that fall on Monday. This perfectly implements the law that January 2 is only a holiday when January 1 falls on Sunday.

Applied to files:

  • holidays/countries/chile.py
📚 Learning: 2025-07-02T18:17:53.342Z
Learnt from: KJhellico
PR: vacanza/holidays#2608
File: tests/countries/test_saint_vincent_and_the_grenadines.py:162-178
Timestamp: 2025-07-02T18:17:53.342Z
Learning: In the Saint Vincent and the Grenadines holidays implementation, New Year's Day is added without observed rules using `_add_new_years_day()` and should not include observed rule testing in its test method. Only holidays explicitly wrapped with `_add_observed()` have observed behavior.

Applied to files:

  • holidays/countries/chile.py
📚 Learning: 2025-07-09T20:27:37.760Z
Learnt from: KJhellico
PR: vacanza/holidays#2623
File: holidays/countries/christmas_island.py:110-112
Timestamp: 2025-07-09T20:27:37.760Z
Learning: In Christmas Island, ANZAC Day (April 25) follows the same observed holiday rules as other holidays, using the SAT_SUN_TO_NEXT_MON rule to move to Monday when it falls on a weekend. The implementation correctly uses `_add_observed` wrapper around `_add_anzac_day`.

Applied to files:

  • holidays/countries/chile.py
🧬 Code graph analysis (1)
holidays/countries/chile.py (1)
holidays/observed_holiday_base.py (1)
  • _add_observed (144-196)
⏰ 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: Test docs build
  • GitHub Check: Build distribution
🔇 Additional comments (4)
holidays/countries/chile.py (4)

121-121: Good improvement to the code documentation.

Adding the "National Holiday" comment provides clear context for the observed holiday rule, aligning with the library's standard practice of including holiday name comments above tr() calls.


175-177: Excellent refactoring to eliminate duplication.

The introduction of the name variable for "Fiestas Patrias" effectively centralizes the translation string, reducing maintenance overhead and ensuring consistency across all uses of this holiday name in the method.


180-180: Consistent application of the refactored variable.

Good work using the centralized name variable across all three holiday declarations (Sep 17 observed, Sep 20 observed, and Sep 20 direct). This maintains consistency and reduces the risk of translation mismatches.

Also applies to: 184-184, 186-186


178-187: Verified — Sep 20 observed logic unchanged.
holidays/countries/chile.py adds the Sep 20 observed holiday only for self._year >= 2008 (the inner guard inside the >=2007 block) and preserves the historical 1932–1944 Sep 20; tests in tests/countries/test_chile.py include expected dates (e.g., 2013-09-20, 2019-09-20, 2024-09-20). No change required.

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/chile.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 (89269c1).
⚠️ Report is 3 commits behind head on dev.

Additional details and impacted files
@@            Coverage Diff            @@
##               dev     #2934   +/-   ##
=========================================
  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
Merged via the queue into vacanza:dev with commit 68d5af4 Sep 16, 2025
36 checks passed
@PPsyrius PPsyrius deleted the chile_refactor branch September 17, 2025 02:59
@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浏览器服务,不要输入任何密码和下载