Add Cocos Islands holidays #2532
Conversation
Signed-off-by: tr33k <devarajkaliyamoorthy@gmail.com>
…runal' Signed-off-by: tr33k <devarajkaliyamoorthy@gmail.com>
Co-authored-by: ~Jhellico <KJhellico@users.noreply.github.com> Signed-off-by: Devaraj K <devarajkaliyamoorthy@gmail.com>
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com> Signed-off-by: Devaraj K <devarajkaliyamoorthy@gmail.com>
Signed-off-by: Devaraj K <devarajkaliyamoorthy@gmail.com>
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com> Signed-off-by: Devaraj K <devarajkaliyamoorthy@gmail.com>
Signed-off-by: tr33k <devarajkaliyamoorthy@gmail.com>
Signed-off-by: tr33k <devarajkaliyamoorthy@gmail.com>
Signed-off-by: tr33k <devarajkaliyamoorthy@gmail.com>
Summary by CodeRabbit
Summary by CodeRabbit
WalkthroughSupport for Cocos (Keeling) Islands holidays was added. This includes a new country module with holiday logic, localization files for multiple languages, updates to the country registry, and comprehensive tests. Documentation and initialization files were updated to reflect the new country. Changes
Assessment against linked issues
Suggested reviewers
📜 Recent review detailsConfiguration used: .coderabbit.yaml 📒 Files selected for processing (1)
🧰 Additional context used🧠 Learnings (1)tests/countries/test_cocos_islands.py (4)🔇 Additional comments (5)
✨ Finishing Touches
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. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
Documentation and Community
|
|
Closes #2426 |
There was a problem hiding this comment.
Actionable comments posted: 6
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (15)
README.md(2 hunks)holidays/calendars/hindu.py(3 hunks)holidays/countries/__init__.py(1 hunks)holidays/countries/cocos_islands.py(1 hunks)holidays/countries/india.py(1 hunks)holidays/groups/hindu.py(4 hunks)holidays/locale/coa_CC/LC_MESSAGES/CC.po(1 hunks)holidays/locale/en_AU/LC_MESSAGES/CC.po(1 hunks)holidays/locale/en_IN/LC_MESSAGES/IN.po(2 hunks)holidays/locale/en_US/LC_MESSAGES/CC.po(1 hunks)holidays/locale/en_US/LC_MESSAGES/IN.po(2 hunks)holidays/locale/hi/LC_MESSAGES/IN.po(2 hunks)holidays/registry.py(1 hunks)tests/countries/test_cocos_islands.py(1 hunks)tests/countries/test_india.py(5 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (5)
holidays/countries/__init__.py (1)
holidays/countries/cocos_islands.py (3)
CocosIslands(43-131)CC(209-212)CCK(215-218)
holidays/countries/india.py (1)
holidays/groups/hindu.py (3)
_add_pongal(259-269)_add_thiruvalluvar_day(321-335)_add_uzhavar_thirunal(337-349)
tests/countries/test_india.py (2)
holidays/countries/india.py (1)
India(28-499)tests/common.py (2)
assertHolidayName(195-199)assertNoHolidayName(273-275)
tests/countries/test_cocos_islands.py (2)
tests/common.py (8)
TestCase(28-338)CommonCountryTests(356-374)assertAliases(121-130)assertNoHolidays(292-294)assertHolidayName(195-199)assertNoNonObservedHoliday(248-250)assertNoHolidayName(273-275)assertLocalizedHolidays(327-338)holidays/countries/cocos_islands.py (3)
CocosIslands(43-131)CC(209-212)CCK(215-218)
holidays/groups/hindu.py (1)
holidays/calendars/hindu.py (1)
pongal_date(1312-1313)
🔇 Additional comments (23)
holidays/locale/en_US/LC_MESSAGES/IN.po (2)
17-17: Version bump verified.Project version updated from 0.69 to 0.72, which aligns with the overall changes for adding new holidays.
311-317: Appropriate localization entries added for Tamil Nadu holidays.The new entries for "Thiruvalluvar Day / Mattu Pongal" and "Uzhavar Thirunal" are correctly added to support the Tamil Nadu holiday implementation. The translations follow the same format as other entries in the file.
holidays/locale/en_IN/LC_MESSAGES/IN.po (2)
17-17: Version bump verified.Project version updated from 0.69 to 0.72, matching the version update in other localization files.
310-316: Appropriate localization entries added for Tamil Nadu holidays.The new entries for "Thiruvalluvar Day / Mattu Pongal" and "Uzhavar Thirunal" have been added with empty msgstr fields, which is consistent with the pattern used for other holidays in this locale file.
holidays/locale/hi/LC_MESSAGES/IN.po (2)
17-17: Version bump verified.Project version updated from 0.69 to 0.72, consistent with other localization files.
310-316: Hindi translations provided for Tamil Nadu holidays.The translations for "Thiruvalluvar Day / Mattu Pongal" (तिरुवल्लुवर दिवस / मट्टू पोंगल) and "Uzhavar Thirunal" (उझावर थिरुनल) are accurate transliterations, following the same pattern as other holiday translations in the file.
holidays/countries/india.py (1)
459-463: Tamil Nadu holiday implementation improved.The changes replace the generic
_add_makar_sankranti()method with the more specific_add_pongal()for Tamil Nadu, and add related holidays "Thiruvalluvar Day / Mattu Pongal" and "Uzhavar Thirunal". These implementations correctly use the new Hindu calendar methods that calculate these dates based on Pongal with appropriate day offsets.From the provided code snippets, I can see that:
_add_pongal()uses the Hindu calendar's pongal_date_add_thiruvalluvar_day()adds 1 day to the Pongal date_add_uzhavar_thirunal()adds 2 days to the Pongal dateThis is culturally accurate and reflects the proper sequence of Tamil Nadu's Pongal festival celebrations.
tests/countries/test_india.py (3)
149-152: Well-implemented Tamil Nadu holiday datesThe addition of "2018-01-15" and "2018-01-16" to the Tamil Nadu subdivision test data properly extends testing coverage for the newly added Thiruvalluvar Day and Uzhavar Thirunal holidays.
398-423: Good test coverage for new Tamil Nadu holidaysWell-structured test blocks for the Thiruvalluvar Day / Mattu Pongal and Uzhavar Thirunal holidays. The tests thoroughly verify:
- Correct dates across multiple years (2001-2035)
- Proper subdivision filtering (present only in TN)
- Consistency with the underlying calendar implementation
The implementation follows the established pattern of other subdivision-specific holiday tests.
532-533: Complete localization testing for new holidaysThe localization tests properly include the new Tamil Nadu holidays across all three locales (default, Hindi, and US English), ensuring translations are correctly applied and retrievable.
Also applies to: 600-601, 663-664
holidays/calendars/hindu.py (3)
38-38: Good addition of PONGAL constantThe PONGAL constant is appropriately added to the list of holiday constants, maintaining alphabetical ordering among the holiday identifiers.
869-905: Well-implemented Pongal dates mappingThe PONGAL_DATES dictionary correctly maps years from 2001 to 2035 to the appropriate January dates (either 14th or 15th). The implementation follows the established pattern of other holiday date mappings in this file.
The dates align with the expected pattern for Pongal, which typically falls on January 14th or 15th based on the solar calendar.
1312-1313: Simple and effective pongal_date implementationThe pongal_date method follows the consistent pattern of other date retrieval methods, using the internal _get_holiday helper with the appropriate holiday identifier.
holidays/groups/hindu.py (4)
31-31: Good enhancement with days_delta parameterThe addition of the days_delta parameter to _add_hindu_calendar_holiday with a default value of 0 maintains backward compatibility while enabling date adjustments for related holidays. This is a clean solution that avoids code duplication.
Also applies to: 41-42
259-270: Well-documented Pongal implementationThe _add_pongal method is well-implemented with a comprehensive docstring explaining the significance of Pongal, its timing, and providing a reference to additional information. The implementation correctly uses the pongal_date method from the Hindu calendar.
321-336: Good implementation of Thiruvalluvar Day / Mattu PongalThe _add_thiruvalluvar_day method effectively uses the days_delta parameter to set the date one day after Pongal. The docstring thoroughly explains both aspects of this dual holiday and includes appropriate references.
337-350: Well-structured Uzhavar Thirunal implementationThe _add_uzhavar_thirunal method correctly implements this holiday two days after Pongal using the days_delta parameter. The documentation clearly explains the significance of the farmers' festival and includes a reference for further information.
holidays/registry.py (1)
57-57: Add Cocos Islands to country registry.Entry
"cocos_islands": ("CocosIslands", "CC", "CCK")is correctly placed in alphabetical order and matches the module and class names.holidays/countries/__init__.py (1)
49-49: Import Cocos Islands country module.The import
from .cocos_islands import CocosIslands, CC, CCKis correctly inserted in alphabetical order and includes all aliases.README.md (1)
382-387: Update supported countries table.The Cocos Islands row is correctly added with code
CCand supported languagescoa_CC,en_AU,en_US. The supported country count is updated to 171.holidays/locale/en_US/LC_MESSAGES/CC.po (1)
30-86: Validate English translations.All
msgstrentries accurately reflect the Cocos Islands holiday names in en_US.holidays/locale/coa_CC/LC_MESSAGES/CC.po (1)
1-86: New coa_CC locale templates.The PO file includes all relevant
msgidentries and placeholdermsgstrfields for the Cocos Islands locale. Translation team can now populate the localized strings.holidays/countries/cocos_islands.py (1)
86-89: Confirm removal of Islamic New Year after 2019You deliberately skip adding Islamic New Year from 2020 onward:
if self._year < 2020: self._add_islamic_new_year_day(...)Is this an official policy change? If not, the holiday will silently disappear in future years.
Please double-check the source and, if required, extendHIJRI_NEW_YEAR_DATESor drop the condition.
Signed-off-by: Devaraj K <devarajkaliyamoorthy@gmail.com>
There was a problem hiding this comment.
Actionable comments posted: 2
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (1)
holidays/countries/cocos_islands.py(1 hunks)
🧰 Additional context used
🪛 Ruff (0.8.2)
holidays/countries/cocos_islands.py
13-13: datetime.timedelta imported but unused
Remove unused import: datetime.timedelta
(F401)
17-17: holidays.utils.nth_weekday_of_month imported but unused
Remove unused import: holidays.utils.nth_weekday_of_month
(F401)
🔇 Additional comments (4)
holidays/countries/cocos_islands.py (4)
44-126: Holiday implementation looks correct and comprehensiveThe implementation covers all expected holidays for the Cocos Islands with proper handling of observed rules and conditional logic based on year ranges. The code is well-structured with clear holiday name translations.
128-200: Islamic holiday date mapping is thoroughThe custom Islamic holiday date mappings are comprehensive, covering multiple years from 2007 to 2026, which ensures accurate holiday calculations for these important dates.
203-212: Alias classes are correctly implementedThe alias classes
CCandCCKfor the Cocos Islands follow the standard pattern used throughout the holidays library.
105-106:❌ Incorrect review comment
Consider using the imported
nth_weekday_of_monthutilityThe code uses
self.nth_weekday_of_month()which appears to be a method inherited from a parent class. For consistency with other holiday modules, consider using the imported utility function directly.- dt = self.nth_weekday_of_month(2, MON, JUN) + dt = nth_weekday_of_month(self._year, JUN, MON, 2)
🏁 Script executed:
#!/bin/bash # Check how other holiday modules use the nth_weekday_of_month utility rg -A 3 "nth_weekday_of_month\(" --glob "holidays/countries/*.py" | head -10Length of output: 678
🏁 Script executed:
#!/bin/bash # Locate the definition/import of nth_weekday_of_month in the codebase rg -n "nth_weekday_of_month" -A2Length of output: 3730
🏁 Script executed:
#!/bin/bash # Show the definition and docstring of nth_weekday_of_month in holidays/utils.py rg -n "def nth_weekday_of_month" -A5 holidays/utils.pyLength of output: 56
🏁 Script executed:
#!/bin/bash # Locate utility files under holidays/utils find holidays -maxdepth 3 -type f | grep -i "utils"Length of output: 69
🏁 Script executed:
#!/bin/bash # Search for nth_weekday_of_month definition in holidays/utils.py grep -R "def nth_weekday_of_month" -n holidays/utils.py || true # Fallback: locate any reference lines to confirm signature grep -R "nth_weekday_of_month" -n holidays/utils.pyLength of output: 116
🏁 Script executed:
#!/bin/bash # Display the contents of utils.py to locate the nth_weekday_of_month definition or import sed -n '1,200p' holidays/utils.pyLength of output: 7576
🏁 Script executed:
#!/bin/bash # Determine file length and inspect lines 200-400 for nth_weekday_of_month wc -l holidays/utils.py sed -n '200,400p' holidays/utils.pyLength of output: 6989
Ignore suggestion to use
nth_weekday_of_monthfromholidays.utilsThe function
nth_weekday_of_monthisn’t defined inholidays/utils.py, so the import inholidays/countries/cocos_islands.pyis invalid. You can continue using the existingself.nth_weekday_of_month(...)or, if you prefer a module‐level utility, import the correct helper (_get_nth_weekday_of_monthfromholidays.calendars.gregorian).Likely an incorrect or invalid review comment.
Signed-off-by: Devaraj K <devarajkaliyamoorthy@gmail.com>
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
holidays/countries/cocos_islands.py (1)
17-17: 🧹 Nitpick (assertive)Remove the unused import.
The static analysis correctly flags that
nth_weekday_of_monthis imported but never directly used in this file. Your code at line 105 usesself.nth_weekday_of_month()method which is inherited from a parent class.-from holidays.utils import nth_weekday_of_monthThis aligns with previous feedback in this codebase where method names like
self.nth_weekday_of_month()use calendar constants internally through parent class implementations even when their imports don't appear to be directly used in the file.🧰 Tools
🪛 Ruff (0.8.2)
17-17:
holidays.utils.nth_weekday_of_monthimported but unusedRemove unused import:
holidays.utils.nth_weekday_of_month(F401)
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (1)
holidays/countries/cocos_islands.py(1 hunks)
🧰 Additional context used
🧠 Learnings (1)
holidays/countries/cocos_islands.py (1)
Learnt from: Wasif-Shahzad
PR: vacanza/holidays#2409
File: holidays/countries/qatar.py:27-46
Timestamp: 2025-04-03T16:58:27.175Z
Learning: In the holidays library, method names like `_add_holiday_2nd_tue_of_feb()` and `_add_holiday_1st_sun_of_mar()` use calendar constants internally through parent class implementations even when these constants don't appear directly in the file. Removing imports that seem unused based on a simple text search could break functionality.
🧬 Code Graph Analysis (1)
holidays/countries/cocos_islands.py (6)
holidays/calendars/islamic.py (1)
_CustomIslamicHolidays(3881-3882)holidays/groups/christian.py (5)
ChristianHolidays(22-463)_add_good_friday(308-317)_add_easter_monday(259-268)_add_christmas_day(208-216)_add_christmas_day_two(218-226)holidays/groups/international.py (3)
InternationalHolidays(18-220)_add_new_years_day(126-134)_next_year_new_years_day(24-28)holidays/groups/islamic.py (5)
IslamicHolidays(20-426)_add_islamic_new_year_day(283-295)_add_eid_al_fitr_day(152-163)_add_eid_al_adha_day(109-120)_add_mawlid_day(329-339)holidays/observed_holiday_base.py (1)
ObservedHolidayBase(101-243)holidays/holiday_base.py (2)
pop(1170-1199)_add_holiday(819-831)
🪛 Ruff (0.8.2)
holidays/countries/cocos_islands.py
17-17: holidays.utils.nth_weekday_of_month imported but unused
Remove unused import: holidays.utils.nth_weekday_of_month
(F401)
🔇 Additional comments (5)
holidays/countries/cocos_islands.py (5)
74-75: Code looks good for New Year's Day handling.The implementation correctly adds New Year's Day with observation rules, including handling the edge case for next year's New Year's Day when it falls on a weekend.
118-125: Good implementation of Boxing Day observed rules.The code correctly handles the special cases for Boxing Day's observed rules based on which day of the week it falls:
- When it falls on Monday, it's moved to Tuesday
- When it falls on Saturday or Sunday, it's moved to Monday or Tuesday
This properly implements the expected behavior for Boxing Day.
203-212: Appropriate use of alias classes.The CC and CCK alias classes follow the standard pattern used in the holidays library to provide convenient access via country/territory codes.
44-126: Overall implementation looks solid.The CocosIslands class is well-structured and follows the pattern established in other country modules. The holiday definitions are comprehensive, and the implementation handles observed holiday rules correctly.
112-115:❓ Verification inconclusive
Christmas Day appears to be added twice.
You're adding Christmas Day with both
_add_observed()and_add_christmas_day(). This may lead to duplicate entries for the same holiday.
🏁 Script executed:
#!/bin/bash # Check if Christmas Day appears twice in the holiday list for Cocos Islands # Run a simple test that creates a holidays instance for 2023 and checks Christmas python -c " import holidays from datetime import date # Create holidays for Cocos Islands h = holidays.CC(years=2023) # Check Christmas Day (2023-12-25) christmas = date(2023, 12, 25) if christmas in h: print(f'Christmas Day found: {h.get(christmas)}') print(f'Count of entries: {len(h.get_list(christmas))}') if len(h.get_list(christmas)) > 1: print('WARNING: Christmas appears multiple times') print(h.get_list(christmas)) else: print('Christmas Day not found') "Length of output: 2277
Fix ImportError in Cocos Islands module
The Cocos Islands country class is failing to load due to a missing utility function, so we can’t verify the Christmas‐day duplication yet.• File holidays/countries/cocos_islands.py, line 17:
from holidays.utils import nth_weekday_of_month– Remove or update this import to match the current utilities API.
Once the import error is resolved, rerun your test to check whether Christmas Day is indeed being added twice.
Signed-off-by: Devaraj K <devarajkaliyamoorthy@gmail.com>
Signed-off-by: Devaraj K <devarajkaliyamoorthy@gmail.com>
There was a problem hiding this comment.
Actionable comments posted: 4
♻️ Duplicate comments (4)
holidays/countries/cocos_islands.py (4)
149-152: 🧹 Nitpick (assertive)Add an explanatory comment for Islamic New Year year limit
The code restricts Islamic New Year to years up to 2019, but there's no explanation why. It would be helpful to add a comment explaining this restriction.
if self._year <= 2019: + # Islamic New Year was only observed as a public holiday until 2019 # Islamic New Year. for dt in self._add_islamic_new_year_day(tr("Islamic New Year")): self._add_observed(dt)
138-142:⚠️ Potential issueRemove the redundant
MON_TO_NEXT_TUErule for Boxing DayThe combination of
SAT_SUN_TO_NEXT_MON_TUE + MON_TO_NEXT_TUEcan cause duplicate "(observed)" labels in the final holiday name (e.g., "Christmas Day (observed) (observed)"). TheSAT_SUN_TO_NEXT_MON_TUErule already handles moving consecutive holidays to Monday and Tuesday when they fall on weekends.self._add_observed( # Boxing Day. self._add_christmas_day_two(tr("Boxing Day")), - rule=SAT_SUN_TO_NEXT_MON_TUE + MON_TO_NEXT_TUE, + rule=SAT_SUN_TO_NEXT_MON_TUE, )
168-236: 🧹 Nitpick (assertive)Add a docstring for the CocosIslandsIslamicHolidays class
This class provides explicit Gregorian date mappings for Islamic holidays, but lacks a docstring explaining its purpose and fallback behavior for years not explicitly mapped.
class CocosIslandsIslamicHolidays(_CustomIslamicHolidays): + """ + Explicit Gregorian date mappings for Islamic holidays in Cocos Islands. + + For years not explicitly defined in the mappings below, dates are calculated + using standard Islamic calendar algorithms from the parent class. + """ HIJRI_NEW_YEAR_DATES = { # ...
102-108: 🧹 Nitpick (assertive)Consider inlining the translation call for consistency
Most holiday declarations in this file directly inline the translation call within the method, but here a variable is assigned first. Consider adopting a consistent approach.
- name = tr("Act of Self Determination Day") - if self._year in act_of_self_determination_dates: - self._add_holiday(name, act_of_self_determination_dates[self._year]) - else: - dt = self._add_holiday_apr_6(name) + if self._year in act_of_self_determination_dates: + self._add_holiday(tr("Act of Self Determination Day"), act_of_self_determination_dates[self._year]) + else: + dt = self._add_holiday_apr_6(tr("Act of Self Determination Day")) if self._year != 2019: self._add_observed(dt)
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (1)
holidays/countries/cocos_islands.py(1 hunks)
🧰 Additional context used
🧠 Learnings (1)
holidays/countries/cocos_islands.py (3)
Learnt from: Wasif-Shahzad
PR: vacanza/holidays#2409
File: holidays/countries/qatar.py:27-46
Timestamp: 2025-04-03T16:58:27.175Z
Learning: In the holidays library, method names like `_add_holiday_2nd_tue_of_feb()` and `_add_holiday_1st_sun_of_mar()` use calendar constants internally through parent class implementations even when these constants don't appear directly in the file. Removing imports that seem unused based on a simple text search could break functionality.
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"))
Learnt from: KJhellico
PR: #2354
File: holidays/countries/fiji.py:63-70
Timestamp: 2025-03-23T10:11:50.465Z
Learning: In the holidays library, the SAT_SUN_TO_NEXT_MON_TUE rule is specifically used for consecutive holidays (like Christmas Day and Boxing Day) to ensure they're observed on separate weekdays (Monday and Tuesday) when they fall on weekends, while SAT_SUN_TO_NEXT_MON is used as the default rule for other holidays.
</details>
</details>
</details>
<details>
<summary>🔇 Additional comments (5)</summary><blockquote>
<details>
<summary>holidays/countries/cocos_islands.py (5)</summary>
`1-13`: **The license header is consistent with other project files**
The license header properly includes the copyright information and MIT license reference, maintaining consistency with the rest of the project.
---
`34-61`: **Thorough documentation with comprehensive references**
The class documentation includes detailed references to official sources for holiday definitions across multiple years, which is excellent for maintainability and verification.
---
`62-74`: **Country configuration is well-structured**
The country code, language settings, and localization labels are properly defined. The start year is appropriately set to 1985 following the Act of Self Determination in 1984.
---
`75-89`: **Constructor implementation is correct**
The constructor properly initializes all parent classes and sets default parameters. The docstring clearly explains the `islamic_show_estimated` parameter.
---
`247-262`: **Well-structured static holidays class with proper references**
The `CocosIslandsStaticHolidays` class is well-organized with separate dictionaries for regular special holidays and observed special holidays. The reference for the National Day of Mourning is properly included.
</details>
</blockquote></details>
</details>
<!-- This is an auto-generated comment by CodeRabbit for review status -->
Signed-off-by: Devaraj K <devarajkaliyamoorthy@gmail.com>
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (8)
holidays/countries/cocos_islands.py (8)
138-142:⚠️ Potential issueFix duplicate "(observed)" labels by removing redundant rule.
Using both
SAT_SUN_TO_NEXT_MON_TUEandMON_TO_NEXT_TUEcauses duplication in observed labels, particularly for consecutive holidays. TheSAT_SUN_TO_NEXT_MON_TUErule alone already handles the Monday-Tuesday separation.self._add_observed( # Boxing Day. self._add_christmas_day_two(tr("Boxing Day")), - rule=SAT_SUN_TO_NEXT_MON_TUE + MON_TO_NEXT_TUE, + rule=SAT_SUN_TO_NEXT_MON_TUE, )
149-152: 🧹 Nitpick (assertive)Add explanation for the Islamic New Year year restriction.
The code only includes Islamic New Year for years up to 2019, but doesn't explain why. A comment would clarify this restriction.
if self._year <= 2019: + # Islamic New Year was only observed as a public holiday until 2019 # Islamic New Year. for dt in self._add_islamic_new_year_day(tr("Islamic New Year")): self._add_observed(dt)
13-28: 🧹 Nitpick (assertive)Remove unused SAT_TO_PREV_FRI import.
The
SAT_TO_PREV_FRIconstant is imported but never used in the implementation. Clean imports improve readability and prevent confusion.from holidays.observed_holiday_base import ( ObservedHolidayBase, MON_TO_NEXT_TUE, SAT_SUN_TO_NEXT_MON, SAT_SUN_TO_NEXT_MON_TUE, - SAT_TO_PREV_FRI, )
98-100: 🧹 Nitpick (assertive)Add a clarifying comment for the special 2007 date.
The Act of Self Determination Day normally falls on April 6, but has a special date in 2007. A brief comment would help explain why.
act_of_self_determination_dates = { + # Special date for 2007 per official proclamation }
107-108: 🧹 Nitpick (assertive)Add explanation for special 2019 handling.
The code skips adding observed rule for Act of Self Determination Day in 2019, but there's no explanation why. Based on line 260, there's a specific observed date defined.
dt = self._add_holiday_apr_6(name) if self._year != 2019: + # Skip adding observed rule for 2019 as a specific observed date is + # defined in special_public_holidays_observed (April 10, 2019) self._add_observed(dt)
168-169: 🛠️ Refactor suggestionAdd class documentation to explain the custom Islamic holiday mappings.
Add a docstring to explain the purpose of this class and how dates are determined for years outside the explicit mappings.
class CocosIslandsIslamicHolidays(_CustomIslamicHolidays): + """ + Explicit Gregorian date mappings for Islamic holidays in Cocos Islands. + For years not explicitly defined, dates are calculated using standard + Islamic calendar algorithms from the parent class. + """ EID_AL_ADHA_DATES = {
119-135: 🧹 Nitpick (assertive)Simplify the Sovereign's Birthday implementation.
The current implementation is a bit verbose. Consider simplifying for better readability.
-name = ( - # King's Birthday. - tr("King's Birthday") - if self._year >= 2023 - # Queen's Birthday. - else tr("Queen's Birthday") -) -if self._year in queens_kings_birthday_dates: - self._add_holiday(name, queens_kings_birthday_dates[self._year]) -else: - self._add_holiday_2nd_mon_of_jun(name) +holiday_name = tr("King's Birthday") if self._year >= 2023 else tr("Queen's Birthday") +if self._year in queens_kings_birthday_dates: + self._add_holiday(holiday_name, queens_kings_birthday_dates[self._year]) +else: + self._add_holiday_2nd_mon_of_jun(holiday_name)
164-165: 🧹 Nitpick (assertive)Add explanation for special 2025 Eid al-Adha handling.
There's special handling for Eid al-Adha in 2025, but no explanation is provided. Based on line 261, there's a specific observed date defined.
for dt in self._add_eid_al_adha_day(tr("Eid al-Adha")): if self._year != 2025: + # Skip adding observed rule for 2025 as a specific observed date is + # defined in special_public_holidays_observed (June 6, 2025) self._add_observed(dt)
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (1)
holidays/countries/cocos_islands.py(1 hunks)
🧰 Additional context used
🧠 Learnings (1)
holidays/countries/cocos_islands.py (3)
Learnt from: Wasif-Shahzad
PR: vacanza/holidays#2409
File: holidays/countries/qatar.py:27-46
Timestamp: 2025-04-03T16:58:27.175Z
Learning: In the holidays library, method names like `_add_holiday_2nd_tue_of_feb()` and `_add_holiday_1st_sun_of_mar()` use calendar constants internally through parent class implementations even when these constants don't appear directly in the file. Removing imports that seem unused based on a simple text search could break functionality.
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"))
Learnt from: KJhellico
PR: #2354
File: holidays/countries/fiji.py:63-70
Timestamp: 2025-03-23T10:11:50.465Z
Learning: In the holidays library, the SAT_SUN_TO_NEXT_MON_TUE rule is specifically used for consecutive holidays (like Christmas Day and Boxing Day) to ensure they're observed on separate weekdays (Monday and Tuesday) when they fall on weekends, while SAT_SUN_TO_NEXT_MON is used as the default rule for other holidays.
</details>
</details><details>
<summary>🪛 Ruff (0.8.2)</summary>
<details>
<summary>holidays/countries/cocos_islands.py</summary>
187-187: Blank line contains whitespace
Remove whitespace from blank line
(W293)
---
206-206: Blank line contains whitespace
Remove whitespace from blank line
(W293)
---
218-218: Blank line contains whitespace
Remove whitespace from blank line
(W293)
</details>
</details>
</details>
<details>
<summary>🔇 Additional comments (2)</summary><blockquote>
<details>
<summary>holidays/countries/cocos_islands.py (2)</summary>
`65-70`: **Well-implemented localization support with estimated and observed labels.**
Your implementation of observed, estimated, and observed_estimated labels properly supports Islamic holidays that might have estimated dates.
---
`247-262`: **Well-implemented static holidays class with proper documentation.**
The static holidays implementation includes good documentation with references and properly handles special cases like the National Day of Mourning and observed holidays.
</details>
</blockquote></details>
</details>
<!-- This is an auto-generated comment by CodeRabbit for review status -->
Co-authored-by: ~Jhellico <KJhellico@users.noreply.github.com> Signed-off-by: Devaraj K <devarajkaliyamoorthy@gmail.com>
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (6)
tests/countries/test_cocos_islands.py (1)
13-18: 🛠️ Refactor suggestionConsider using only CommonCountryTests as the base class.
The
TestCocosIslandsclass is inheriting from bothCommonCountryTestsandTestCase, butCommonCountryTestsalready extendsTestCase. This creates unnecessary inheritance.-from unittest import TestCase - from holidays.countries.cocos_islands import CocosIslands, CC, CCK from tests.common import CommonCountryTests -class TestCocosIslands(CommonCountryTests, TestCase): +class TestCocosIslands(CommonCountryTests):holidays/countries/cocos_islands.py (5)
168-233: 🧹 Nitpick (assertive)Add docstring to CocosIslandsIslamicHolidays class.
The
CocosIslandsIslamicHolidaysclass provides detailed date mappings but lacks documentation explaining its purpose and behavior for years outside the explicit mappings.class CocosIslandsIslamicHolidays(_CustomIslamicHolidays): + """ + Explicit Gregorian date mappings for Islamic holidays in Cocos Islands. + For years not explicitly defined, dates are calculated using standard + Islamic calendar algorithms from the parent class. + """ EID_AL_ADHA_DATES = {
137-142:⚠️ Potential issueFix duplicate "(observed)" labels in Boxing Day rule.
Using both
SAT_SUN_TO_NEXT_MON_TUEandMON_TO_NEXT_TUEfor Boxing Day can cause duplicate "(observed)" labels (e.g., "Christmas Day (observed) (observed)" on 2022-12-27). TheSAT_SUN_TO_NEXT_MON_TUErule already handles the Monday-Tuesday separation for consecutive holidays.self._add_observed( # Boxing Day. self._add_christmas_day_two(tr("Boxing Day")), - rule=SAT_SUN_TO_NEXT_MON_TUE + MON_TO_NEXT_TUE, + rule=SAT_SUN_TO_NEXT_MON_TUE, )
163-165: 🧹 Nitpick (assertive)Add explanatory comment for Eid al-Adha 2025 special case.
The code skips adding the observed rule for Eid al-Adha in 2025, but doesn't explain why.
# Eid al-Adha. for dt in self._add_eid_al_adha_day(tr("Eid al-Adha")): if self._year != 2025: + # Skip adding observed rule for 2025 as a specific observed date is + # defined in special_public_holidays_observed (June 6, 2025) self._add_observed(dt)
98-108: 🛠️ Refactor suggestionAdd comments explaining Act of Self Determination Day special cases.
The code handles special cases for Act of Self Determination Day in 2007 and 2019, but lacks explanatory comments.
act_of_self_determination_dates = { + # Special date for 2007 per official proclamation } # Act of Self Determination Day. name = tr("Act of Self Determination Day") if self._year in act_of_self_determination_dates: self._add_holiday(name, act_of_self_determination_dates[self._year]) else: dt = self._add_holiday_apr_6(name) if self._year != 2019: + # Skip adding observed rule for 2019 as a specific observed date is + # defined in special_public_holidays_observed (April 10, 2019) self._add_observed(dt)
149-152: 🧹 Nitpick (assertive)Add explanatory comment for Islamic New Year year limit.
The code limits Islamic New Year to years up to 2019, but doesn't explain why.
if self._year <= 2019: + # Islamic New Year was only observed as a public holiday until 2019 # Islamic New Year. for dt in self._add_islamic_new_year_day(tr("Islamic New Year")): self._add_observed(dt)
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (2)
holidays/countries/cocos_islands.py(1 hunks)tests/countries/test_cocos_islands.py(1 hunks)
🧰 Additional context used
🧠 Learnings (2)
tests/countries/test_cocos_islands.py (4)
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.
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.
Learnt from: KJhellico
PR: vacanza/holidays#2532
File: tests/countries/test_cocos_islands.py:78-89
Timestamp: 2025-05-12T15:31:58.051Z
Learning: In the holidays project, tests for movable holidays (like Easter Monday) should always use static date sets only, not calculation functions.
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.
holidays/countries/cocos_islands.py (3)
Learnt from: Wasif-Shahzad
PR: vacanza/holidays#2409
File: holidays/countries/qatar.py:27-46
Timestamp: 2025-04-03T16:58:27.175Z
Learning: In the holidays library, method names like `_add_holiday_2nd_tue_of_feb()` and `_add_holiday_1st_sun_of_mar()` use calendar constants internally through parent class implementations even when these constants don't appear directly in the file. Removing imports that seem unused based on a simple text search could break functionality.
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"))
Learnt from: KJhellico
PR: #2354
File: holidays/countries/fiji.py:63-70
Timestamp: 2025-03-23T10:11:50.465Z
Learning: In the holidays library, the SAT_SUN_TO_NEXT_MON_TUE rule is specifically used for consecutive holidays (like Christmas Day and Boxing Day) to ensure they're observed on separate weekdays (Monday and Tuesday) when they fall on weekends, while SAT_SUN_TO_NEXT_MON is used as the default rule for other holidays.
</details>
</details><details>
<summary>🧬 Code Graph Analysis (1)</summary>
<details>
<summary>tests/countries/test_cocos_islands.py (2)</summary><blockquote>
<details>
<summary>tests/common.py (9)</summary>
* `TestCase` (28-338)
* `CommonCountryTests` (356-374)
* `assertAliases` (121-130)
* `assertNoHolidays` (292-294)
* `assertHoliday` (150-152)
* `assertHolidayName` (195-199)
* `assertNoNonObservedHoliday` (248-250)
* `assertNoHolidayName` (273-275)
* `assertLocalizedHolidays` (327-338)
</details>
<details>
<summary>holidays/countries/cocos_islands.py (3)</summary>
* `CocosIslands` (31-165)
* `CC` (236-237)
* `CCK` (240-241)
</details>
</blockquote></details>
</details>
</details>
<details>
<summary>🔇 Additional comments (8)</summary><blockquote>
<details>
<summary>tests/countries/test_cocos_islands.py (5)</summary>
`21-24`: **The test setup is comprehensive.**
Great job setting up tests with both regular and non-observed holiday instances, as well as a special instance for Islamic holidays without estimated labels.
---
`32-35`: **Test for special holidays is clear and focused.**
Good job implementing a dedicated test for the National Day of Mourning for Queen Elizabeth II special holiday.
---
`118-138`: **Sovereign's Birthday test is well-implemented.**
The test correctly handles the transition from Queen's Birthday to King's Birthday in 2023, checking both specific dates and the default calculation for other years.
---
`154-175`: **Islamic New Year test is properly configured.**
Good job using the `no_estimated_holidays` instance for testing Islamic holidays and including the correct date range (only up to 2019).
---
`243-262`: **Localization tests are thorough and well-structured.**
The localization tests cover all three supported languages (default, Cocos Malay, and US English) with a complete set of 2022 holidays. Great job ensuring translations are verified for all holidays.
Also applies to: 264-284, 286-306
</details>
<details>
<summary>holidays/countries/cocos_islands.py (3)</summary>
`31-73`: **The class setup and metadata is well-structured.**
The `CocosIslands` class correctly extends the necessary base classes and includes comprehensive documentation with numerous references. The metadata properties (country code, languages, labels, etc.) are appropriately defined.
---
`75-89`: **Constructor implementation is clean and follows best practices.**
The constructor properly initializes all parent classes and provides a clear parameter for controlling Islamic holiday estimated labels.
---
`244-259`: **Special holidays implementation is clean and well-structured.**
The `CocosIslandsStaticHolidays` class correctly implements both regular special holidays and special observed holidays with appropriate documentation.
</details>
</blockquote></details>
</details>
<!-- This is an auto-generated comment by CodeRabbit for review status -->
Signed-off-by: tr33k <devarajkaliyamoorthy@gmail.com>
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
tests/countries/test_cocos_islands.py (1)
13-19: 🧹 Nitpick (assertive)Consider removing redundant TestCase inheritance.
CommonCountryTests already extends TestCase, making the additional TestCase import and inheritance redundant.
-from unittest import TestCase - from holidays.countries.cocos_islands import CocosIslands, CC, CCK from tests.common import CommonCountryTests -class TestCocosIslands(CommonCountryTests, TestCase): +class TestCocosIslands(CommonCountryTests):
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (1)
tests/countries/test_cocos_islands.py(1 hunks)
🧰 Additional context used
🧠 Learnings (1)
tests/countries/test_cocos_islands.py (4)
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.
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.
Learnt from: KJhellico
PR: vacanza/holidays#2532
File: tests/countries/test_cocos_islands.py:78-89
Timestamp: 2025-05-12T15:31:58.051Z
Learning: In the holidays project, tests for movable holidays (like Easter Monday) should always use static date sets only, not calculation functions.
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.
🔇 Additional comments (6)
tests/countries/test_cocos_islands.py (6)
80-92: Good implementation of Good Friday test with static dates.Following the project convention of using static date sets for movable holidays rather than calculation functions.
93-104: Good implementation of Easter Monday test with static dates.The test correctly includes the 2024 date that was missing in previous versions and follows the project convention of using static date sets.
116-136: Well-structured test for sovereign's birthday transition.The test properly handles the transition from Queen's Birthday to King's Birthday in 2023, testing both specific dates and date ranges.
152-174: Well-structured test for Islamic New Year.The test correctly uses the no_estimated_holidays instance to verify dates and includes the proper end date for this holiday (observed until 2019).
241-260: Comprehensive default localization test.The test thoroughly verifies all holiday names in the default locale, including compound holidays like "Boxing Day; Christmas Day (observed)" on 2022-12-26.
262-282: Good Cocos Malay localization implementation.The test properly verifies holiday names in Cocos Malay, including correct translation of "observed" to "disambut".
Signed-off-by: Devaraj K <devarajkaliyamoorthy@gmail.com>
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (4)
tests/countries/test_cocos_islands.py (4)
13-19: 🧹 Nitpick (assertive)Remove redundant TestCase inheritance.
CommonCountryTestsalready extendsunittest.TestCase, so you can drop the explicitTestCaseimport and removeTestCasefrom the class bases for consistency.-from unittest import TestCase - -class TestCocosIslands(CommonCountryTests, TestCase): +class TestCocosIslands(CommonCountryTests):
32-35: 🛠️ Refactor suggestionAssert the holiday name for special holidays.
Instead of only checking the date, use
assertHolidayNameto verify that"2022-09-22"is indeed"National Day of Mourning for Queen Elizabeth II".def test_special_holidays(self): - self.assertHoliday( - "2022-09-22", - ) + self.assertHolidayName( + "National Day of Mourning for Queen Elizabeth II", + "2022-09-22", + )
80-92: 🛠️ Refactor suggestionRemove dynamic range assertion for Good Friday.
Per project convention, tests for movable holidays must list explicit dates only. Drop the
range(1985, 2050)assertion.- self.assertHolidayName(name, range(1985, 2050))
93-105: 🛠️ Refactor suggestionRemove dynamic range assertion for Easter Monday.
Similarly, retain only the hard-coded dates and remove the
range(1985, 2050)call intest_easter_monday.- self.assertHolidayName(name, range(1985, 2050))
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (1)
tests/countries/test_cocos_islands.py(1 hunks)
🧰 Additional context used
🧠 Learnings (1)
tests/countries/test_cocos_islands.py (4)
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.
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.
Learnt from: KJhellico
PR: vacanza/holidays#2532
File: tests/countries/test_cocos_islands.py:78-89
Timestamp: 2025-05-12T15:31:58.051Z
Learning: In the holidays project, tests for movable holidays (like Easter Monday) should always use static date sets only, not calculation functions.
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.
|
@KJhellico Do the tests need to include the observed |
For Christmas Day tests you just don't need |
|
And I would like to highlight once again: we usually use recent years in our tests (this applies to observed and especially Islamic holidays). |
Signed-off-by: tr33k <devarajkaliyamoorthy@gmail.com>
Co-authored-by: ~Jhellico <KJhellico@users.noreply.github.com> Signed-off-by: Devaraj K <devarajkaliyamoorthy@gmail.com>
|
Signed-off-by: tr33k <devarajkaliyamoorthy@gmail.com> Signed-off-by: Devaraj K <devarajkaliyamoorthy@gmail.com> Co-authored-by: ~Jhellico <KJhellico@users.noreply.github.com> Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
Signed-off-by: tr33k <devarajkaliyamoorthy@gmail.com> Signed-off-by: Devaraj K <devarajkaliyamoorthy@gmail.com> Co-authored-by: ~Jhellico <KJhellico@users.noreply.github.com> Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
Proposed change
Add Cocos Islands holidays.
Resolves #2426.
Type of change
holidaysfunctionality in general)Checklist
make check, all checks and tests are green