Update documentation: clarify subdivisions_aliases note#3205
Merged
arkid15r merged 5 commits intoMar 21, 2026
Conversation
Contributor
Summary by CodeRabbit
WalkthroughREADME documentation updated: clarified that Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Suggested reviewers
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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. Comment |
|
Contributor
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
README.md (1)
118-124: Fix grammar issues in the language handling section.A few grammar issues need attention:
- Line 119: Missing verb - should be "when neither user-specified language nor user locale language is available"
- Line 119: Missing hyphen - "user specified" should be "user-specified"
- Line 118: Optional improvement - "Some of the countries" could be simplified to "Some countries"
- Line 122: Minor - "by country" would be clearer as "by a country" or "by each country"
📝 Suggested fixes
-Some of the countries support more than one language for holiday names output. A default language is defined by `default_language` (optional) attribute for each +Some countries support more than one language for holiday names output. A default language is defined by the `default_language` (optional) attribute for each entity and is used as a fallback when neither user specified language nor user locale language -available. The default language code is a [ISO 639-1 +is available. The default language code is an [ISO 639-1 code](https://en.wikipedia.org/wiki/List_of_ISO_639-1_codes). A list of all languages supported by -country is defined by `supported_languages` (optional) attribute. If there is no designated [ISO +each country is defined by the `supported_languages` (optional) attribute. If there is no designated [ISO 639-1 code](https://en.wikipedia.org/wiki/List_of_ISO_639-1_codes) then [ISO 639-2 code](https://en.wikipedia.org/wiki/List_of_ISO_639-2_codes) can be used.Note: Also fixed article consistency ("a" → "an" before "ISO" and "the" before attribute names).
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (1)
README.md
🧰 Additional context used
🧠 Learnings (9)
📓 Common learnings
Learnt from: PPsyrius
Repo: vacanza/holidays PR: 2642
File: holidays/countries/france.py:178-182
Timestamp: 2025-06-18T10:18:59.447Z
Learning: In the France holidays implementation, deprecated subdivision names like "Alsace-Moselle" and "Saint-Barthélémy" are handled through explicit conditionals in _populate_public_holidays() that map them to their corresponding new subdivision-specific holiday population methods, providing backward compatibility while users transition to the new ISO codes.
Learnt from: PPsyrius
Repo: vacanza/holidays PR: 3026
File: holidays/countries/spain.py:189-189
Timestamp: 2025-10-28T17:26:45.090Z
Learning: In the vacanza/holidays codebase, `_populate` methods (such as `_populate_public_holidays`, `_populate_subdiv_holidays`, and subdivision-specific methods like `_populate_subdiv_XX_public_holidays`) intentionally do not include return type annotations like `-> None`. This is a consistent pattern across country implementations (e.g., Germany, United States, Spain) and should not be changed even when static analysis tools like Ruff suggest adding them.
<!-- </add_learning>
Learnt from: KJhellico
Repo: vacanza/holidays PR: 2651
File: tests/countries/test_bonaire_sint_eustatius_and_saba.py:225-306
Timestamp: 2025-07-02T10:22:33.053Z
Learning: The `assertLocalizedHolidays` method in the vacanza/holidays project requires a complete list of all holidays from all subdivisions, not just subdivision-specific holidays. This is a framework requirement for proper localization testing.
Learnt from: PPsyrius
Repo: vacanza/holidays PR: 2881
File: tests/countries/test_faroe_islands.py:16-16
Timestamp: 2025-11-08T05:36:32.788Z
Learning: In PR #2881 onwards, the vacanza/holidays project moved to centralized alias testing via the `check_aliases` method in `tests/common.py`. Individual country test files no longer need to import country code aliases (e.g., FO, FRO for Faroe Islands) or define `test_country_aliases` methods. The common test framework automatically validates all aliases by dynamically importing them from the registry and calling `assertAliases`.
Learnt from: KJhellico
Repo: vacanza/holidays PR: 2606
File: README.md:562-568
Timestamp: 2025-06-06T16:02:09.910Z
Learning: The README.md country table displays ISO 3166-1 alpha-2 codes only in the "Code" column. Alpha-3 codes or country aliases should not be included in this table format, maintaining consistency with all other country entries.
Learnt from: KJhellico
Repo: vacanza/holidays PR: 2757
File: README.md:1592-1597
Timestamp: 2025-07-26T13:30:58.728Z
Learning: In the holidays project README.md, supported languages in the country table are always arranged in alphabetical order, with the default language marked in bold but maintaining its alphabetical position rather than being moved to a special position.
📚 Learning: 2025-07-26T13:30:58.728Z
Learnt from: KJhellico
Repo: vacanza/holidays PR: 2757
File: README.md:1592-1597
Timestamp: 2025-07-26T13:30:58.728Z
Learning: In the holidays project README.md, supported languages in the country table are always arranged in alphabetical order, with the default language marked in bold but maintaining its alphabetical position rather than being moved to a special position.
Applied to files:
README.md
📚 Learning: 2025-06-16T11:46:35.303Z
Learnt from: PPsyrius
Repo: vacanza/holidays PR: 2638
File: holidays/countries/svalbard_and_jan_mayen.py:26-32
Timestamp: 2025-06-16T11:46:35.303Z
Learning: When creating country holiday aliases that inherit from parent countries (like Svalbard and Jan Mayen from Norway, or Åland from Finland), it's common and acceptable to hardcode a specific subdivision code since the subdivision codes for the parent country yield identical results.
Applied to files:
README.md
📚 Learning: 2025-06-16T11:46:35.303Z
Learnt from: PPsyrius
Repo: vacanza/holidays PR: 2638
File: holidays/countries/svalbard_and_jan_mayen.py:26-32
Timestamp: 2025-06-16T11:46:35.303Z
Learning: When creating country holiday aliases that inherit from parent countries (like Svalbard and Jan Mayen from Norway, or Åland from Finland), it's standard practice to hardcode a specific subdivision code in the _populate methods since the subdivision codes for the parent country yield identical results. The Åland Islands implementation serves as the reference pattern for this approach.
Applied to files:
README.md
📚 Learning: 2025-06-18T10:07:58.780Z
Learnt from: PPsyrius
Repo: vacanza/holidays PR: 2642
File: holidays/countries/french_southern_territories.py:41-44
Timestamp: 2025-06-18T10:07:58.780Z
Learning: Territorial holiday classes that inherit from parent countries (like HolidaysAX from Finland, HolidaysSJ from Norway, HolidaysTF from France) follow a standard pattern of silently overriding self.subdiv in their _populate_public_holidays() method without validation, as this ensures they always use the correct subdivision code for their territory regardless of user input.
Applied to files:
README.md
📚 Learning: 2025-06-18T10:18:59.447Z
Learnt from: PPsyrius
Repo: vacanza/holidays PR: 2642
File: holidays/countries/france.py:178-182
Timestamp: 2025-06-18T10:18:59.447Z
Learning: In the France holidays implementation, deprecated subdivision names like "Alsace-Moselle" and "Saint-Barthélémy" are handled through explicit conditionals in _populate_public_holidays() that map them to their corresponding new subdivision-specific holiday population methods, providing backward compatibility while users transition to the new ISO codes.
Applied to files:
README.md
📚 Learning: 2025-06-16T11:46:35.303Z
Learnt from: PPsyrius
Repo: vacanza/holidays PR: 2638
File: holidays/countries/svalbard_and_jan_mayen.py:26-32
Timestamp: 2025-06-16T11:46:35.303Z
Learning: In the holidays library, Norwegian subdivisions all yield the same holiday results regardless of the subdivision code used. This is similar to how Åland is implemented as a simple alias of Finland holidays with just a country code override.
Applied to files:
README.md
📚 Learning: 2025-06-18T10:12:57.419Z
Learnt from: PPsyrius
Repo: vacanza/holidays PR: 2642
File: holidays/countries/france.py:178-182
Timestamp: 2025-06-18T10:12:57.419Z
Learning: In the France holidays implementation, subdivisions like "GES" and "Metropole" that covered multiple departments with different holiday rules are completely deprecated rather than aliased, because aliasing them to a subset (like Alsace-Moselle) would provide incomplete or incorrect holiday data for the other covered regions. Complete deprecation is preferred over partial/incorrect data coverage.
Applied to files:
README.md
📚 Learning: 2025-03-26T08:55:08.917Z
Learnt from: PPsyrius
Repo: vacanza/holidays PR: 2362
File: holidays/ical.py:53-80
Timestamp: 2025-03-26T08:55:08.917Z
Learning: The holidays library intentionally limits language code support to ISO 639-1 and ISO 639-2 standards (2-3 letter language codes with optional region) to maintain consistency with other modules like `common_holidays` and `financial_holidays`, rather than implementing full RFC 5646 compliance.
Applied to files:
README.md
🔇 Additional comments (1)
README.md (1)
107-117: Good clarification for users.The note effectively communicates that
subdivisions_aliasesis not exhaustive and that valid subdivisions may exist without being listed. The formatting and emphasis are appropriate.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## dev #3205 +/- ##
=========================================
Coverage 100.00% 100.00%
=========================================
Files 310 310
Lines 18580 18580
Branches 2379 2379
=========================================
Hits 18580 18580 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
PPsyrius
reviewed
Jan 30, 2026
Contributor
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
README.md (1)
117-123:⚠️ Potential issue | 🟡 MinorTighten grammar in the language fallback paragraph.
Current wording is a bit hard to parse (e.g., “neither user specified language nor user locale language available”). A small rewrite will make fallback behavior unambiguous.
✏️ Suggested wording
-Some of the countries support more than one language for holiday names output. A default language is defined by `default_language` (optional) attribute for each -entity and is used as a fallback when neither user specified language nor user locale language -available. The default language code is a [ISO 639-1 +Some countries support more than one language for holiday name output. A default language is defined by the optional `default_language` attribute for each +entity and is used as a fallback when neither the user-specified language nor the user locale language is available. The default language code is an [ISO 639-1 code](https://en.wikipedia.org/wiki/List_of_ISO_639-1_codes). A list of all languages supported by -country is defined by `supported_languages` (optional) attribute. If there is no designated [ISO +country is defined by the optional `supported_languages` attribute. If there is no designated [ISO 639-1 code](https://en.wikipedia.org/wiki/List_of_ISO_639-1_codes) then [ISO 639-2 code](https://en.wikipedia.org/wiki/List_of_ISO_639-2_codes) can be used.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@README.md` around lines 117 - 123, The paragraph explaining language fallback is awkwardly worded; rewrite it for clarity by explicitly stating the fallback order and simplifying phrases: mention that each entity may define an optional default_language (ISO 639-1, fallback if neither the user-specified language nor the user's locale language is available) and an optional supported_languages list; clarify that if an ISO 639-1 code is not available an ISO 639-2 code may be used, and replace the current confusing clause ("neither user specified language nor user locale language available") with a clear sequence like "use user-specified language → else user locale language → else default_language."
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@README.md`:
- Around line 117-123: The paragraph explaining language fallback is awkwardly
worded; rewrite it for clarity by explicitly stating the fallback order and
simplifying phrases: mention that each entity may define an optional
default_language (ISO 639-1, fallback if neither the user-specified language nor
the user's locale language is available) and an optional supported_languages
list; clarify that if an ISO 639-1 code is not available an ISO 639-2 code may
be used, and replace the current confusing clause ("neither user specified
language nor user locale language available") with a clear sequence like "use
user-specified language → else user locale language → else default_language."
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: c236435b-8efe-4022-bf49-fc198195fb25
📒 Files selected for processing (1)
README.md
|
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
README.md (1)
109-109:⚠️ Potential issue | 🟡 MinorUse standard GitHub Alert capitalization.
The alert type should be all caps per GitHub's standard format. While case-insensitive in practice, the convention is
[!NOTE]rather than[!Note].📝 Suggested fix
-> [!Note] +> [!NOTE]🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@README.md` at line 109, Change the GitHub alert token from "[!Note]" to the uppercase convention "[!NOTE]" in the README; locate the markdown line containing the alert token (the string "[!Note]") and replace it with "[!NOTE]" to follow standard GitHub Alert capitalization.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@README.md`:
- Around line 117-119: Update the README sentence that explains the fallback
language: change "when neither user specified language nor user locale language
available" to "when neither the user-specified language nor the user locale
language is available" and change "a [ISO 639-1" to "an [ISO 639-1" so the
phrase around the default_language attribute reads grammatically correct; locate
the sentence referencing the default_language attribute and apply these wording
fixes.
---
Duplicate comments:
In `@README.md`:
- Line 109: Change the GitHub alert token from "[!Note]" to the uppercase
convention "[!NOTE]" in the README; locate the markdown line containing the
alert token (the string "[!Note]") and replace it with "[!NOTE]" to follow
standard GitHub Alert capitalization.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: b61c2f13-35c6-4a11-bf16-1243820bb48a
📒 Files selected for processing (1)
README.md
arkid15r
approved these changes
Mar 21, 2026
Devika9705
pushed a commit
to Devika9705/holidays
that referenced
this pull request
Mar 22, 2026
Co-authored-by: Arkadii Yakovets <ark@cho.red> Co-authored-by: Arkadii Yakovets <2201626+arkid15r@users.noreply.github.com>
Merged
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
4 participants
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.
This PR updates the README to clarify the usage of the
subdivisions_aliasesattribute.subdivisions_aliases.This is a documentation-only change.