Update Guinea-Bissau holidays #2859
Merged
Conversation
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
Summary by CodeRabbit
WalkthroughRemoved two observed-related class attributes from the GuineaBissau holiday class and deleted corresponding translation entries from en_US and pt_GW GW.po files; PO metadata was updated. A test was added to assert observed_label implies ObservedHolidayBase usage. Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Assessment against linked issues
Assessment against linked issues: Out-of-scope changes
Possibly related PRs
Suggested labels
Suggested reviewers
Tip 🔌 Remote MCP (Model Context Protocol) integration is now available!Pro plan users can now connect to remote MCP servers from the Integrations page. Connect with popular remote MCPs such as Notion and Linear to add more context to your reviews and chats. ✨ 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 #2859 +/- ##
=========================================
Coverage 100.00% 100.00%
=========================================
Files 290 290
Lines 17235 17233 -2
Branches 2242 2242
=========================================
- Hits 17235 17233 -2 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
There was a problem hiding this comment.
Actionable comments posted: 1
📜 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 (1)
tests/common.py(2 hunks)
🧰 Additional context used
🧠 Learnings (13)
📓 Common learnings
Learnt from: KJhellico
PR: vacanza/holidays#2854
File: holidays/locale/en_US/LC_MESSAGES/SD.po:38-42
Timestamp: 2025-08-25T22:19:01.304Z
Learning: In the holidays library, translator comments and translations should consistently use the "observed, estimated" order pattern. Guinea-Bissau (GW.po) was identified as having an inconsistent "estimated, observed" comment that needs to be fixed to match the established convention.
Learnt from: KJhellico
PR: vacanza/holidays#2854
File: holidays/locale/en_US/LC_MESSAGES/SD.po:38-42
Timestamp: 2025-08-25T22:19:01.305Z
Learning: In the holidays library, translator comments and translations should consistently use the "observed, estimated" order pattern. Guinea-Bissau (GW.po) was identified as having an inconsistent "estimated, observed" comment that needs to be fixed to match the established convention.
Learnt from: KJhellico
PR: vacanza/holidays#2398
File: holidays/countries/guinea.py:106-110
Timestamp: 2025-04-04T10:52:41.546Z
Learning: In the Guinea holidays implementation, observed Eid al-Fitr cases are properly covered by the test_eid_al_fitr_day() method, which tests both the regular holiday dates and the observed dates when the holiday falls on a non-working day (for years >= 2023).
Learnt from: KJhellico
PR: vacanza/holidays#2402
File: holidays/countries/trinidad_and_tobago.py:85-92
Timestamp: 2025-04-25T20:27:59.086Z
Learning: The `_populate_observed` method in holiday classes should maintain the same signature as the parent class `ObservedHolidayBase`, even if specific child class implementations don't use all parameters.
Learnt from: KJhellico
PR: vacanza/holidays#2398
File: holidays/countries/guinea.py:106-110
Timestamp: 2025-04-04T10:52:41.546Z
Learning: In the Guinea holidays implementation, observed Eid al-Fitr cases are covered by the test_eid_al_fitr_day() method, which tests both regular holiday dates and the observed dates when the holiday falls on a non-working day (for years >= 2023).
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.
Learnt from: KJhellico
PR: vacanza/holidays#2623
File: tests/countries/test_christmas_island.py:136-146
Timestamp: 2025-07-09T21:16:35.145Z
Learning: In Christmas Island's holiday implementation, the test_christmas_day method cannot use assertNoNonObservedHoliday because in some years observed Christmas Day overlaps with Boxing Day when both holidays are moved due to weekend conflicts, causing the standard non-observed holiday check to fail inappropriately.
Learnt from: PPsyrius
PR: vacanza/holidays#2489
File: holidays/countries/sao_tome_and_principe.py:22-26
Timestamp: 2025-04-23T14:55:35.504Z
Learning: References in holidays classes should only be included if they're used for test case cross-checks or provide historical context about when holidays were added/removed, not just for the sake of having more references.
Learnt from: PPsyrius
PR: vacanza/holidays#2489
File: holidays/countries/sao_tome_and_principe.py:22-26
Timestamp: 2025-04-23T14:55:35.504Z
Learning: References in holidays classes should only be included if they're used for test case cross-checks or provide historical context about when holidays were added/removed, not just for the sake of having more references.
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-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/common.py
📚 Learning: 2025-08-20T19:46:15.595Z
Learnt from: KJhellico
PR: vacanza/holidays#2833
File: tests/countries/test_uganda.py:15-0
Timestamp: 2025-08-20T19:46:15.595Z
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/common.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: When adding a new country in vacanza/holidays, also re-export it in holidays/countries/__init__.py (e.g., from .liberia import Liberia, LR, LBR) so tests and users can import from holidays.countries consistently.
Applied to files:
tests/common.py
📚 Learning: 2025-08-09T18:31:23.218Z
Learnt from: KJhellico
PR: vacanza/holidays#2778
File: tests/countries/test_kiribati.py:15-15
Timestamp: 2025-08-09T18:31:23.218Z
Learning: In the holidays project test files, the standard import pattern is `from holidays.countries.<country> import Country, CODE1, CODE2` (used in ~97% of test files), not `from holidays.countries import Country, CODE1, CODE2`. Only a few exceptions (Suriname, Sierra Leone, Mauritius, Mali, Ivory Coast, Guyana) use the aggregated import pattern.
Applied to files:
tests/common.py
📚 Learning: 2025-04-05T04:33:53.254Z
Learnt from: PPsyrius
PR: vacanza/holidays#2416
File: tests/countries/test_turkmenistan.py:52-64
Timestamp: 2025-04-05T04:33:53.254Z
Learning: For Turkmenistan holiday tests, recommend using CommonCountryTests as the base class rather than unittest.TestCase to follow project conventions, be consistent with other country test files, and gain access to common test utilities.
Applied to files:
tests/common.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/common.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:
tests/common.py
📚 Learning: 2025-04-05T04:50:40.752Z
Learnt from: PPsyrius
PR: vacanza/holidays#2416
File: tests/countries/test_turkmenistan.py:31-49
Timestamp: 2025-04-05T04:50:40.752Z
Learning: For Turkmenistan holiday tests, use this class structure: `class TestTurkmenistan(CommonCountryTests, TestCase)` with imports `from unittest import TestCase`, `from holidays.countries import Turkmenistan, TM, TKM`, and `from tests.common import CommonCountryTests`. Ensure to call `super().setUp()` in the setUp method.
Applied to files:
tests/common.py
📚 Learning: 2025-04-05T04:47:27.213Z
Learnt from: PPsyrius
PR: vacanza/holidays#2416
File: tests/countries/test_turkmenistan.py:52-64
Timestamp: 2025-04-05T04:47:27.213Z
Learning: For holiday tests in the vacanza/holidays project, structure tests by individual holidays rather than by years. Each test method should focus on a specific holiday and test it across multiple years (from start_year through 2050) using helper methods like `assertHolidayName`. For fixed holidays, use generators like `(f"{year}-01-01" for year in range(1991, 2051))`. For movable holidays, specify individual dates for specific years followed by a range check.
Applied to files:
tests/common.py
📚 Learning: 2025-05-06T21:07:11.577Z
Learnt from: KJhellico
PR: vacanza/holidays#2530
File: tests/countries/test_andorra.py:23-28
Timestamp: 2025-05-06T21:07:11.577Z
Learning: In the holidays project, test methods for country holidays follow a standard form where year ranges are explicitly recreated in each test method rather than being stored as class variables, to maintain consistency across all country tests.
Applied to files:
tests/common.py
📚 Learning: 2025-03-29T17:55:09.799Z
Learnt from: ankushhKapoor
PR: vacanza/holidays#2386
File: holidays/countries/nepal.py:29-34
Timestamp: 2025-03-29T17:55:09.799Z
Learning: In the holidays library, classes with multiple inheritance from various holiday groups (like ChristianHolidays, HinduCalendarHolidays, etc.) should initialize each parent class separately rather than using `super().__init__(*args, **kwargs)` because the parent classes have different parameter requirements. HolidayBase should receive the `*args, **kwargs` while other holiday group classes typically don't accept parameters like `observed`.
Applied to files:
tests/common.py
📚 Learning: 2025-06-15T15:24:53.055Z
Learnt from: KJhellico
PR: vacanza/holidays#2606
File: holidays/countries/faroe_islands.py:62-67
Timestamp: 2025-06-15T15:24:53.055Z
Learning: The `HolidayBase` class uses `__getattr__` to dynamically implement `_add_holiday_*` methods through pattern matching, including patterns like `_add_holiday_<n>_days_past_easter`, `_add_holiday_<month>_<day>`, and various weekday-relative patterns. Methods like `_add_holiday_26_days_past_easter` are not explicitly defined but are dynamically generated when called.
Applied to files:
tests/common.py
🧬 Code graph analysis (1)
tests/common.py (1)
holidays/observed_holiday_base.py (1)
ObservedHolidayBase(102-246)
⏰ 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). (3)
- GitHub Check: Test build on ubuntu-latest
- GitHub Check: Test build on macos-latest
- GitHub Check: Test build on windows-latest
🔇 Additional comments (1)
tests/common.py (1)
23-23: Import ObservedHolidayBase for explicit type check — LGTMThe import is minimal and scoped to tests, enabling the new invariant without leaking runtime dependencies.
Comment on lines
+377
to
+387
| def test_observed_label(self): | ||
| if getattr(self.holidays, "observed_label", None): | ||
| self.assertTrue( | ||
| isinstance( | ||
| self.holidays, | ||
| ObservedHolidayBase, | ||
| ), | ||
| "The `observed_label` is not required as this entity doesn't handle " | ||
| "observed holidays.", | ||
| ) | ||
|
|
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Only ObservedHolidayBase entities should expose observed_label — tighten guard and make failure actionable
Good addition; it enforces the intended contract and complements test_observed_estimated_label nicely. Two small improvements:
- Use hasattr instead of getattr to catch empty-string cases.
- Rename the test and make the failure message include the entity name and a clear fix path.
Apply this diff:
-def test_observed_label(self):
- if getattr(self.holidays, "observed_label", None):
- self.assertTrue(
- isinstance(
- self.holidays,
- ObservedHolidayBase,
- ),
- "The `observed_label` is not required as this entity doesn't handle "
- "observed holidays.",
- )
+def test_observed_label_requires_observed_base(self):
+ if hasattr(self.holidays, "observed_label"):
+ self.assertTrue(
+ isinstance(self.holidays, ObservedHolidayBase),
+ (
+ f"The `observed_label` is not required: {self.holidays.__class__.__name__} "
+ "doesn't handle observed holidays. Remove the attribute or subclass "
+ "ObservedHolidayBase."
+ ),
+ )Run this script to catch any remaining entities that define observed_label/observed_estimated_label without inheriting ObservedHolidayBase and to sanity-check the GW.po translator comment order:
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "Scanning for classes that define observed_label* without ObservedHolidayBase…"
python - << 'PY'
import ast, os, sys
root = 'holidays/countries'
bad = []
def base_name(node):
if isinstance(node, ast.Name):
return node.id
if isinstance(node, ast.Attribute):
parts = []
while isinstance(node, ast.Attribute):
parts.append(node.attr)
node = node.value
if isinstance(node, ast.Name):
parts.append(node.id)
return '.'.join(reversed(parts))
return None
for dirpath, _, filenames in os.walk(root):
for fn in filenames:
if not fn.endswith('.py'):
continue
path = os.path.join(dirpath, fn)
try:
with open(path, 'r', encoding='utf-8') as f:
src = f.read()
tree = ast.parse(src, filename=path)
except Exception:
continue
for cls in [n for n in tree.body if isinstance(n, ast.ClassDef)]:
bases = {base_name(b) for b in cls.bases}
inherits_observed = any(b and b.endswith('ObservedHolidayBase') for b in bases)
class_attrs = set()
for n in cls.body:
if isinstance(n, ast.Assign):
for t in n.targets:
if isinstance(t, ast.Name):
class_attrs.add(t.id)
if not inherits_observed and {'observed_label','observed_estimated_label'} & class_attrs:
lines = []
for n in cls.body:
if isinstance(n, ast.Assign):
for t in n.targets:
if isinstance(t, ast.Name) and t.id in ('observed_label','observed_estimated_label'):
lines.append(n.lineno)
bad.append((path, cls.name, sorted(lines)))
if bad:
for path, cls, lines in bad:
print(f'{path}:{",".join(map(str,lines))}: class {cls} defines observed_label* but does not inherit ObservedHolidayBase')
sys.exit(1)
else:
print('OK: no classes define observed_label* without ObservedHolidayBase')
PY
echo
echo "Verifying GW.po translator comment uses 'observed, estimated' and no deprecated order remains…"
if rg -n 'observed, estimated' holidays/locale/en_US/LC_MESSAGES/GW.po > /dev/null; then
echo "OK: en_US GW.po has 'observed, estimated'."
else
echo "FAIL: en_US GW.po missing 'observed, estimated' translator comment." && exit 1
fi
if rg -n 'estimated, observed' holidays/locale/en_US/LC_MESSAGES/GW.po > /dev/null; then
echo "FAIL: Found deprecated 'estimated, observed' order in en_US GW.po." && exit 1
else
echo "OK: No deprecated order in en_US GW.po."
fiLength of output: 704
Update GW.po Translator Comment & Optional Test Refactor
The guard change in test_observed_label is safe—no classes define observed_label/observed_estimated_label without inheriting ObservedHolidayBase. However, the GW.po translator comment still uses the deprecated order and must be corrected:
• In holidays/locale/en_US/LC_MESSAGES/GW.po, replace any occurrence of
“estimated, observed”
with
“observed, estimated”
Example diff (approximate location):
-#. Estimated, observed holidays are recognized…
+#. Observed, estimated holidays are recognized…Without this change, the PO files remain inconsistent with the established “observed, estimated” convention.
Optional: tighten and clarify the test as originally suggested:
-def test_observed_label(self):
- if getattr(self.holidays, "observed_label", None):
- self.assertTrue(
- isinstance(
- self.holidays,
- ObservedHolidayBase,
- ),
- "The `observed_label` is not required as this entity doesn't handle "
- "observed holidays.",
- )
+def test_observed_label_requires_observed_base(self):
+ if hasattr(self.holidays, "observed_label"):
+ self.assertTrue(
+ isinstance(self.holidays, ObservedHolidayBase),
+ (
+ f"The `observed_label` is not required: {self.holidays.__class__.__name__} "
+ "doesn't handle observed holidays. Remove the attribute or subclass "
+ "ObservedHolidayBase."
+ ),
+ )This renames the test for clarity, uses hasattr to catch empty strings, and surfaces the entity name in the failure message.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| def test_observed_label(self): | |
| if getattr(self.holidays, "observed_label", None): | |
| self.assertTrue( | |
| isinstance( | |
| self.holidays, | |
| ObservedHolidayBase, | |
| ), | |
| "The `observed_label` is not required as this entity doesn't handle " | |
| "observed holidays.", | |
| ) | |
| def test_observed_label_requires_observed_base(self): | |
| if hasattr(self.holidays, "observed_label"): | |
| self.assertTrue( | |
| isinstance(self.holidays, ObservedHolidayBase), | |
| ( | |
| f"The `observed_label` is not required: {self.holidays.__class__.__name__} " | |
| "doesn't handle observed holidays. Remove the attribute or subclass " | |
| "ObservedHolidayBase." | |
| ), | |
| ) |
🤖 Prompt for AI Agents
In tests/common.py around lines 377-387, the translator comment in GW.po uses
the deprecated ordering "estimated, observed" and the test could be tightened:
update holidays/locale/en_US/LC_MESSAGES/GW.po to replace any occurrence of
"estimated, observed" with "observed, estimated" to match the project
convention; optionally refactor test_observed_label to rename it for clarity,
use hasattr(self.holidays, "observed_label") (or getattr check for empty string)
and include the entity name in the assertion failure message so the test
surfaces which class failed.
This was referenced Aug 26, 2025
Merged
24 tasks
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
Remove
observed_labelandobserved_estimated_label, as Guinea-Bissau has no observed holidays.Resolves #2855.
Type of change
holidaysfunctionality in general)Checklist
make checklocally; all checks and tests passed.