Expose entity additional codes#2879
Merged
Merged
Conversation
Contributor
Summary by CodeRabbit
WalkthroughRefactors EntityLoader._get_entity_codes to accept explicit min/max code lengths and iterate only over codes after the human-readable name; include_aliases now controls yielding either only the first alias or all aliases. get_country_codes and get_financial_codes updated to call the new signature. Tests added for alias modes. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Suggested reviewers
📜 Recent review detailsConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro 💡 Knowledge Base configuration:
You can enable these sources in your CodeRabbit configuration. 📒 Files selected for processing (1)
🧰 Additional context used🧠 Learnings (16)📚 Learning: 2025-08-08T10:33:55.695ZApplied to files:
📚 Learning: 2025-08-24T13:01:51.370ZApplied to files:
📚 Learning: 2025-04-18T21:13:55.589ZApplied to files:
📚 Learning: 2025-08-24T13:01:51.370ZApplied to files:
📚 Learning: 2025-08-03T12:59:53.286ZApplied to files:
📚 Learning: 2025-06-09T19:50:56.039ZApplied to files:
📚 Learning: 2025-06-24T19:41:46.854ZApplied to files:
📚 Learning: 2025-06-05T17:22:43.114ZApplied to files:
📚 Learning: 2025-04-02T04:51:17.842ZApplied to files:
📚 Learning: 2025-08-08T14:37:03.045ZApplied to files:
📚 Learning: 2025-07-15T15:00:32.728ZApplied to files:
📚 Learning: 2025-08-28T21:03:22.907ZApplied to files:
📚 Learning: 2025-08-24T11:55:06.810ZApplied to files:
📚 Learning: 2025-07-17T11:07:04.986ZApplied to files:
📚 Learning: 2025-08-20T19:46:15.625ZApplied to files:
📚 Learning: 2025-08-26T20:10:05.288ZApplied to files:
⏰ 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)
🔇 Additional comments (2)
✨ Finishing Touches
🧪 Generate unit tests
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
Status, Documentation and Community
|
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## dev #2879 +/- ##
=========================================
Coverage 100.00% 100.00%
=========================================
Files 294 294
Lines 17445 17444 -1
Branches 2262 2262
=========================================
- Hits 17445 17444 -1 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Contributor
There was a problem hiding this comment.
Actionable comments posted: 4
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (2)
holidays/registry.py(2 hunks)tests/test_registry.py(1 hunks)
🧰 Additional context used
🧠 Learnings (1)
📓 Common learnings
Learnt from: KJhellico
PR: vacanza/holidays#2763
File: README.md:108-110
Timestamp: 2025-08-08T10:33:55.695Z
Learning: When verifying the country total in holidays/registry.py, isolate between '^COUNTRIES:' and '^FINANCIAL:' and count only lines matching the strict key pattern '^\s*"[a-z0-9_ ]+":\s*\(' to avoid overcounting non-entry quoted lines. As of PR vacanza/holidays#2763, this yields 227 entries.
Learnt from: KJhellico
PR: vacanza/holidays#2774
File: tests/countries/test_liberia.py:15-16
Timestamp: 2025-08-08T14:37:03.045Z
Learning: In holidays/countries/__init__.py, re-export country classes using absolute imports (e.g., 'from holidays.countries.liberia import Liberia, LR, LBR') and keep alphabetical ordering (e.g., Lesotho, Liberia, Libya). Avoid relative imports in this file.
Learnt from: KJhellico
PR: vacanza/holidays#2483
File: holidays/registry.py:177-177
Timestamp: 2025-04-18T21:13:55.589Z
Learning: In the holidays/registry.py file, COUNTRIES dictionary entries should use the class name (without spaces) as the first element of the tuple (e.g., "TurksAndCaicosIslands"), not the human-readable country name with spaces.
Learnt from: Wasif-Shahzad
PR: vacanza/holidays#2852
File: tests/countries/test_tajikistan.py:26-28
Timestamp: 2025-08-24T13:01:51.370Z
Learning: In the holidays project country test files, the standard method name for testing country aliases (ISO codes) is `test_country_aliases`, not `test_aliases`. This naming convention is consistently used across all country test files.
🧬 Code graph analysis (1)
tests/test_registry.py (1)
holidays/registry.py (3)
EntityLoader(288-382)get_country_codes(353-360)get_financial_codes(363-370)
⏰ 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 (4)
holidays/registry.py (2)
16-16: Typing import cleanup looks good.
Any/Optionalonly; no strayUnion.
367-369: Docstring reads fine.Examples align with financial identifiers.
tests/test_registry.py (2)
111-118: LGTM: baseline behavior pinned for include_aliases=False.Asserts only the primary code (index 1) shows up.
126-140: LGTM: financial code expectations are clear.Both include/exclude cases read correctly.
KJhellico
reviewed
Aug 31, 2025
Contributor
There was a problem hiding this comment.
Actionable comments posted: 4
♻️ Duplicate comments (3)
tests/test_registry.py (1)
119-128: Use code-shaped filter (2–3 uppercase letters) instead of isupper().Aligns with the “codes-only” contract and excludes non-code aliases like HolidaysAX or name-like tokens.
- for entity_classes in registry.COUNTRIES.values(): - self.assertNotIn(entity_classes[0], country_codes) - for code in entity_classes[1:]: - if code.isupper(): - self.assertIn(code, country_codes) - else: - self.assertNotIn(code, country_codes) + for entity_classes in registry.COUNTRIES.values(): + self.assertNotIn(entity_classes[0], country_codes) + for code in entity_classes[1:]: + if code.isalpha() and code.isupper() and len(code) in (2, 3): + self.assertIn(code, country_codes) + else: + self.assertNotIn(code, country_codes)holidays/registry.py (2)
359-366: Docstring: clarify “aliases” means code aliases; exclude non-code names.Be explicit that country “codes” are 2–3 uppercase letters (ISO-style); name-like aliases are not returned.
- :param include_aliases: - Whether to include entity aliases (e.g. GBR and UK for GB, - UKR for UA, USA for US, etc). + :param include_aliases: + Whether to include code aliases (2–3 uppercase letters; e.g., GBR and UK for GB, + UKR for UA, USA for US). Non-code aliases (e.g., 'HolidaysAX', name-like tokens) + are excluded.
363-366: Keep “codes” strictly code-shaped at the API boundary.Filter to 2–3 uppercase alphabetic codes for countries to avoid accidental leakage if data changes.
- return EntityLoader._get_entity_codes( - COUNTRIES, - include_aliases=include_aliases, - ) + return ( + code + for code in EntityLoader._get_entity_codes( + COUNTRIES, include_aliases=include_aliases + ) + if code.isalpha() and code.isupper() and 2 <= len(code) <= 3 + )
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (2)
holidays/registry.py(2 hunks)tests/test_registry.py(1 hunks)
🧰 Additional context used
🧠 Learnings (14)
📚 Learning: 2025-08-24T13:01:51.370Z
Learnt from: Wasif-Shahzad
PR: vacanza/holidays#2852
File: tests/countries/test_tajikistan.py:26-28
Timestamp: 2025-08-24T13:01:51.370Z
Learning: In the holidays project country test files, the standard method name for testing country aliases (ISO codes) is `test_country_aliases`, not `test_aliases`. This naming convention is consistently used across all country test files.
Applied to files:
tests/test_registry.pyholidays/registry.py
📚 Learning: 2025-08-24T13:01:51.370Z
Learnt from: Wasif-Shahzad
PR: vacanza/holidays#2852
File: tests/countries/test_tajikistan.py:26-28
Timestamp: 2025-08-24T13:01:51.370Z
Learning: In the holidays project country test files, the universally adopted standard method name for testing country aliases (ISO codes) is `test_country_aliases`. This naming convention is used consistently across all 235+ country test files in the repository, with no exceptions using `test_aliases`.
Applied to files:
tests/test_registry.pyholidays/registry.py
📚 Learning: 2025-04-18T21:13:55.589Z
Learnt from: KJhellico
PR: vacanza/holidays#2483
File: holidays/registry.py:177-177
Timestamp: 2025-04-18T21:13:55.589Z
Learning: In the holidays/registry.py file, COUNTRIES dictionary entries should use the class name (without spaces) as the first element of the tuple (e.g., "TurksAndCaicosIslands"), not the human-readable country name with spaces.
Applied to files:
tests/test_registry.pyholidays/registry.py
📚 Learning: 2025-08-20T19:46:15.625Z
Learnt from: KJhellico
PR: vacanza/holidays#2833
File: tests/countries/test_uganda.py:15-0
Timestamp: 2025-08-20T19:46:15.625Z
Learning: In the holidays project, the standard import pattern for country test files is `from holidays.countries.<country> import Country, CODE1, CODE2` (direct module import), used by ~75% of country test files. Only a minority (~25%) use the aggregated public API import `from holidays.countries import Country, CODE1, CODE2`. The direct module import pattern should be used for new country test files to follow the established convention.
Applied to files:
tests/test_registry.py
📚 Learning: 2025-06-16T15:48:48.680Z
Learnt from: PPsyrius
PR: vacanza/holidays#2615
File: tests/countries/test_anguilla.py:1-12
Timestamp: 2025-06-16T15:48:48.680Z
Learning: Test files in the holidays repository follow a standardized structure without module or class docstrings. All country test files use the same pattern: license header, imports, and class definition (`class Test{Country}(CommonCountryTests, TestCase):`) without docstrings. This is an established codebase convention that should be maintained for consistency.
Applied to files:
tests/test_registry.py
📚 Learning: 2025-04-05T04:29:38.042Z
Learnt from: PPsyrius
PR: vacanza/holidays#2416
File: tests/countries/test_turkmenistan.py:85-86
Timestamp: 2025-04-05T04:29:38.042Z
Learning: For testing holiday implementations in the vacanza/holidays repository, recommend using `from tests.common import CommonCountryTests` as the base class instead of directly using `unittest.TestCase` to maintain consistency with project conventions and leverage common test utilities.
Applied to files:
tests/test_registry.py
📚 Learning: 2025-06-09T19:50:56.039Z
Learnt from: KJhellico
PR: vacanza/holidays#2609
File: holidays/countries/nauru.py:149-154
Timestamp: 2025-06-09T19:50:56.039Z
Learning: In the holidays library project, never suggest adding docstrings for alias classes (like NR, NRU country code aliases that inherit from main country classes). The project deliberately omits docstrings for these simple alias classes.
Applied to files:
tests/test_registry.pyholidays/registry.py
📚 Learning: 2025-08-08T10:33:55.695Z
Learnt from: KJhellico
PR: vacanza/holidays#2763
File: README.md:108-110
Timestamp: 2025-08-08T10:33:55.695Z
Learning: When verifying the country total in holidays/registry.py, isolate between '^COUNTRIES:' and '^FINANCIAL:' and count only lines matching the strict key pattern '^\s*"[a-z0-9_ ]+":\s*\(' to avoid overcounting non-entry quoted lines. As of PR vacanza/holidays#2763, this yields 227 entries.
Applied to files:
holidays/registry.py
📚 Learning: 2025-08-03T12:59:53.286Z
Learnt from: KJhellico
PR: vacanza/holidays#2635
File: README.md:108-110
Timestamp: 2025-08-03T12:59:53.286Z
Learning: Always verify country counts in the COUNTRIES dictionary by checking the current count in the COUNTRIES dictionary in holidays/registry.py directly, rather than relying on information from previous PRs, as the count changes frequently with new country additions.
Applied to files:
holidays/registry.py
📚 Learning: 2025-06-24T19:41:46.854Z
Learnt from: KJhellico
PR: vacanza/holidays#2651
File: holidays/countries/bonaire_sint_eustatius_and_saba.py:121-126
Timestamp: 2025-06-24T19:41:46.854Z
Learning: In the holidays library, simple alias classes that inherit from parent classes with just `pass` statements do not include docstrings. This is a consistent pattern throughout the codebase and should be maintained for consistency.
Applied to files:
holidays/registry.py
📚 Learning: 2025-06-05T17:22:43.114Z
Learnt from: Wasif-Shahzad
PR: vacanza/holidays#2593
File: holidays/countries/senegal.py:104-110
Timestamp: 2025-06-05T17:22:43.114Z
Learning: In the holidays library codebase, country alias classes (like SN, SEN for Senegal) follow a consistent convention of having no docstrings and just containing "pass". This pattern is used across all countries - examples include BR/BRA for Brazil, CA/CAN for Canada, DE/DEU for Germany, etc. Static analysis tools may flag missing docstrings, but the established codebase convention is to keep these alias classes minimal.
Applied to files:
holidays/registry.py
📚 Learning: 2025-04-02T04:51:17.842Z
Learnt from: PPsyrius
PR: vacanza/holidays#2402
File: holidays/countries/trinidad_and_tobago.py:28-33
Timestamp: 2025-04-02T04:51:17.842Z
Learning: The library convention for country code aliases (like two-letter ISO 3166-1 alpha-2 and three-letter ISO 3166-1 alpha-3 codes) is to implement them as simple pass-through classes without docstrings.
Applied to files:
holidays/registry.py
📚 Learning: 2025-08-08T14:37:03.045Z
Learnt from: KJhellico
PR: vacanza/holidays#2774
File: tests/countries/test_liberia.py:15-16
Timestamp: 2025-08-08T14:37:03.045Z
Learning: In holidays/countries/__init__.py, re-export country classes using absolute imports (e.g., 'from holidays.countries.liberia import Liberia, LR, LBR') and keep alphabetical ordering (e.g., Lesotho, Liberia, Libya). Avoid relative imports in this file.
Applied to files:
holidays/registry.py
📚 Learning: 2025-08-26T20:10:05.288Z
Learnt from: KJhellico
PR: vacanza/holidays#2834
File: holidays/financial/national_stock_exchange_of_india.py:38-44
Timestamp: 2025-08-26T20:10:05.288Z
Learning: For National Stock Exchange of India (NSE) holidays implementation, only `estimated_label = tr("%s (estimated)")` is needed for localization support. The `observed_label` and `observed_estimated_label` are not required for this financial market holidays implementation.
Applied to files:
holidays/registry.py
🧬 Code graph analysis (1)
tests/test_registry.py (1)
holidays/registry.py (3)
EntityLoader(288-392)get_country_codes(356-366)get_financial_codes(369-380)
⏰ 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 (3)
tests/test_registry.py (2)
111-118: Index-based assumption is brittle; break only after yielding first code.This test assumes the first alias (index 1) is always the primary code. It mirrors the current data, but it will silently pass if order ever changes. Consider asserting “exactly one code per entity when include_aliases=False” and let the implementation choose which, or make the implementation break only after yielding a matching code (see registry comment).
129-136: LGTM for “first alias only” behavior.Matches the intended include_aliases=False contract.
holidays/registry.py (1)
16-16: Import cleanup is correct.Removing unused Union is appropriate after the signature change.
|
PPsyrius
approved these changes
Sep 1, 2025
Merged
9 tasks
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
3 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.
Proposed change
Expose entity additional codes. Originally based on this comment.
Type of change
holidaysfunctionality in general)Checklist
make checklocally; all checks and tests passed.