-
-
Notifications
You must be signed in to change notification settings - Fork 558
Refactor Eastern holidays groups #2966
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
Summary by CodeRabbit
WalkthroughRefactors calendar group helpers and imports, adds a batching helper for Eastern calendar holidays, delegates multiple group helpers to it, adjusts one Tibetan method’s return type, formats a Taiwan method signature, and adds a pyproject.toml linter setting. No public exports otherwise changed. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested reviewers
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests
📜 Recent review detailsConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro 📒 Files selected for processing (2)
🧰 Additional context used🧠 Learnings (3)📓 Common learnings
📚 Learning: 2025-04-23T09:59:19.886Z
Applied to files:
📚 Learning: 2025-05-01T19:13:44.664Z
Applied to files:
🧬 Code graph analysis (2)holidays/groups/hebrew.py (3)
holidays/calendars/hebrew.py (2)
⏰ 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)
🔇 Additional comments (1)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 Ruff (0.13.1)holidays/calendars/hebrew.py�[1;31mruff failed�[0m holidays/groups/hebrew.py�[1;31mruff failed�[0m 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 |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## dev #2966 +/- ##
=========================================
Coverage 100.00% 100.00%
=========================================
Files 301 301
Lines 17978 17961 -17
Branches 2328 2319 -9
=========================================
- Hits 17978 17961 -17 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
holidays/groups/hebrew.py (2)
63-67
: Remove# type: ignore[arg-type]
after making helper accept OptionalOnce
_add_hebrew_calendar_holiday
takesOptional[date]
, these ignores are unnecessary noise.Apply:
- return self._add_hebrew_calendar_holiday( - name, - self._hebrew_calendar.lag_baomer_date(self._year), # type: ignore[arg-type] - days_delta, - ) + return self._add_hebrew_calendar_holiday( + name, self._hebrew_calendar.lag_baomer_date(self._year), days_delta + )(Repeat the same edit for Passover, Purim, Rosh Hashanah, Shavuot, Sukkot, Yom Kippur call sites.)
Also applies to: 77-81, 92-95, 106-110, 120-123, 133-137, 147-151
38-52
: Optional: return concrete dates for Hanukkah
_hebrew_calendar.hanukkah_date()
yieldsset[Optional[date]]
. Returning that leaks Nones. Consider filtering toset[date]
for consistency with other helpers.- ) -> set[Optional[date]]: + ) -> set[date]: @@ - dts = self._hebrew_calendar.hanukkah_date(self._year) - for dt in dts: - self._add_hebrew_calendar_holiday(name, dt, days_delta) # type: ignore[arg-type] - return dts + dts = {dt for dt in self._hebrew_calendar.hanukkah_date(self._year) if dt} + for dt in dts: + self._add_hebrew_calendar_holiday(name, dt, days_delta) + return dts
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (11)
holidays/countries/taiwan.py
(1 hunks)holidays/groups/balinese_saka.py
(1 hunks)holidays/groups/buddhist.py
(1 hunks)holidays/groups/eastern.py
(2 hunks)holidays/groups/hebrew.py
(2 hunks)holidays/groups/hindu.py
(2 hunks)holidays/groups/islamic.py
(38 hunks)holidays/groups/mongolian.py
(1 hunks)holidays/groups/sinhala.py
(2 hunks)holidays/groups/tibetan.py
(2 hunks)pyproject.toml
(1 hunks)
🧰 Additional context used
🧠 Learnings (26)
📓 Common learnings
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#2843
File: holidays/countries/burundi.py:15-16
Timestamp: 2025-08-21T05:56:33.276Z
Learning: In the holidays library, when importing Gregorian month constants from holidays.calendars.gregorian, only import the months that are actually used in the date data. For example, if Islamic holiday dates only reference JUN, JUL, SEP, OCT, then only import those specific constants rather than importing additional unused months.
📚 Learning: 2025-04-25T20:27:59.086Z
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.
Applied to files:
holidays/countries/taiwan.py
📚 Learning: 2025-06-14T20:12:37.212Z
Learnt from: KJhellico
PR: vacanza/holidays#2614
File: holidays/countries/guyana.py:146-179
Timestamp: 2025-06-14T20:12:37.212Z
Learning: The `_CustomHinduHolidays` mechanism works through `_CustomCalendarType` metaclass which renames public attributes (like `DIWALI_INDIA_DATES`) with a postfix, allowing `_HinduLunisolar::_get_holiday` to find and use custom holiday dates. When `_add_diwali_india()` is called, it uses the custom dates if available rather than calculated dates.
Applied to files:
holidays/groups/hindu.py
📚 Learning: 2025-04-03T16:58:27.175Z
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.
Applied to files:
holidays/groups/hindu.py
holidays/groups/mongolian.py
holidays/groups/hebrew.py
holidays/groups/sinhala.py
holidays/groups/tibetan.py
holidays/groups/buddhist.py
holidays/groups/balinese_saka.py
holidays/groups/islamic.py
📚 Learning: 2025-08-21T05:56:33.276Z
Learnt from: PPsyrius
PR: vacanza/holidays#2843
File: holidays/countries/burundi.py:15-16
Timestamp: 2025-08-21T05:56:33.276Z
Learning: In the holidays library, when importing Gregorian month constants from holidays.calendars.gregorian, only import the months that are actually used in the date data. For example, if Islamic holiday dates only reference JUN, JUL, SEP, OCT, then only import those specific constants rather than importing additional unused months.
Applied to files:
holidays/groups/hindu.py
holidays/groups/hebrew.py
holidays/groups/islamic.py
📚 Learning: 2025-03-19T16:54:58.657Z
Learnt from: PPsyrius
PR: vacanza/holidays#2354
File: holidays/countries/fiji.py:146-159
Timestamp: 2025-03-19T16:54:58.657Z
Learning: In the holidays library implementation, explicit holiday dates (like Diwali in Fiji) are only defined for historical years with official sources (2016-2025). Future dates beyond the explicitly defined range are automatically calculated by methods like `_add_diwali`, which provide approximations when official dates aren't yet available.
Applied to files:
holidays/groups/hindu.py
📚 Learning: 2025-05-01T19:13:44.664Z
Learnt from: KJhellico
PR: vacanza/holidays#2386
File: holidays/countries/nepal.py:203-208
Timestamp: 2025-05-01T19:13:44.664Z
Learning: Multiple holidays on the same date are properly handled in the holidays library. When multiple `_add_...` methods are called with different holiday names for the same date, both holidays are preserved rather than the latter overwriting the former. This happens because the `__setitem__` method in `HolidayBase` merges multiple holidays on the same date by combining their names.
Applied to files:
holidays/groups/hebrew.py
📚 Learning: 2025-08-11T13:48:45.953Z
Learnt from: PPsyrius
PR: vacanza/holidays#2794
File: holidays/calendars/ethiopian.py:13-13
Timestamp: 2025-08-11T13:48:45.953Z
Learning: The holidays library does not use `__all__` declarations in calendar modules (holidays/calendars/). Calendar files follow a standard pattern of defining constants and functions directly without explicit exports, similar to the convention used in country modules.
Applied to files:
holidays/groups/sinhala.py
holidays/groups/balinese_saka.py
📚 Learning: 2025-09-21T05:16:38.578Z
Learnt from: PPsyrius
PR: vacanza/holidays#2951
File: tests/calendars/test_thai.py:0-0
Timestamp: 2025-09-21T05:16:38.578Z
Learning: In holidays/calendars/thai.py, the `buddhist_sabbath_dates` method in `_ThaiLunisolar` is independent of calendar type (THAI_CALENDAR vs KHMER_CALENDAR) and calculates Buddhist Sabbath dates consistently regardless of which calendar is being used.
Applied to files:
holidays/groups/buddhist.py
📚 Learning: 2025-06-04T10:09:28.732Z
Learnt from: Wasif-Shahzad
PR: vacanza/holidays#2593
File: holidays/calendars/islamic.py:3993-3994
Timestamp: 2025-06-04T10:09:28.732Z
Learning: In the holidays/calendars/islamic.py file, the Islamic calendar date methods in the _IslamicLunar class (like ali_al_rida_death_dates, ashura_dates, etc.) follow a consistent pattern of being single-line methods that return self._get_holiday(CONSTANT, year) without docstrings. New methods should follow this same pattern for consistency.
Applied to files:
holidays/groups/islamic.py
📚 Learning: 2025-03-30T08:08:31.656Z
Learnt from: PPsyrius
PR: vacanza/holidays#2388
File: holidays/countries/ivory_coast.py:84-85
Timestamp: 2025-03-30T08:08:31.656Z
Learning: Islamic holiday logic should be implemented in the holidays/groups/islamic.py file, with country-specific files only calling these methods with the appropriate translated names.
Applied to files:
holidays/groups/islamic.py
📚 Learning: 2025-08-22T19:06:04.303Z
Learnt from: KJhellico
PR: vacanza/holidays#2850
File: holidays/countries/christmas_island.py:75-80
Timestamp: 2025-08-22T19:06:04.303Z
Learning: Christmas Island's docstring for the `islamic_show_estimated` parameter follows the exact same format used consistently across all countries with Islamic holidays in the codebase: "Whether to add 'estimated' label to Islamic holidays name if holiday date is estimated." This is the standard, established pattern and should not be changed.
Applied to files:
holidays/groups/islamic.py
📚 Learning: 2025-06-13T12:18:03.539Z
Learnt from: PPsyrius
PR: vacanza/holidays#2614
File: holidays/countries/guyana.py:78-90
Timestamp: 2025-06-13T12:18:03.539Z
Learning: The holidays codebase now uses the constructor signature pattern `__init__(self, *args, islamic_show_estimated: bool = True, **kwargs)` across country classes.
Applied to files:
holidays/groups/islamic.py
📚 Learning: 2025-08-19T21:00:47.849Z
Learnt from: KJhellico
PR: vacanza/holidays#2831
File: holidays/countries/south_sudan.py:84-88
Timestamp: 2025-08-19T21:00:47.849Z
Learning: In the holidays library, Islamic holidays use dedicated methods for additional days (like `_add_eid_al_fitr_day_two`, `_add_eid_al_adha_day_two`) rather than parameters. The methods don't accept a `days` parameter - each day has its own specific method.
Applied to files:
holidays/groups/islamic.py
📚 Learning: 2025-06-06T14:40:31.932Z
Learnt from: KJhellico
PR: vacanza/holidays#2593
File: holidays/countries/senegal.py:66-110
Timestamp: 2025-06-06T14:40:31.932Z
Learning: In the holidays library, within the _populate_public_holidays method, holidays should be arranged by calendar type (Islamic holidays first, then Gregorian holidays) without additional type grouping comments. The organization by calendar type is sufficient and follows the project's established conventions.
Applied to files:
holidays/groups/islamic.py
📚 Learning: 2025-08-25T04:28:02.061Z
Learnt from: PPsyrius
PR: vacanza/holidays#2848
File: tests/countries/test_somalia.py:44-127
Timestamp: 2025-08-25T04:28:02.061Z
Learning: In the holidays library, Islamic holiday tests use `self.no_estimated_holidays = Country(years=years, islamic_show_estimated=False)` as the library-wide standard approach to simplify test cases. This pattern is intentional and preferred over testing estimated labels.
Applied to files:
holidays/groups/islamic.py
📚 Learning: 2025-04-13T19:10:31.502Z
Learnt from: KJhellico
PR: vacanza/holidays#2465
File: holidays/countries/suriname.py:219-251
Timestamp: 2025-04-13T19:10:31.502Z
Learning: The `_CustomIslamicHolidays` classes in this project contain only exact verified holiday dates from reliable sources, rather than calculated or estimated future dates. This is by design to ensure accuracy, particularly for religious holidays that may follow lunar calendars or depend on local observations.
Applied to files:
holidays/groups/islamic.py
📚 Learning: 2025-04-13T20:41:56.613Z
Learnt from: KJhellico
PR: vacanza/holidays#2386
File: holidays/countries/nepal.py:266-284
Timestamp: 2025-04-13T20:41:56.613Z
Learning: The Islamic holiday dates in the holidays library should only include officially verified dates, not predicted ones, to maintain accuracy. This is especially important for holidays that depend on lunar observations like Eid al-Fitr and Eid al-Adha.
Applied to files:
holidays/groups/islamic.py
📚 Learning: 2025-05-06T13:01:22.693Z
Learnt from: Wasif-Shahzad
PR: vacanza/holidays#2522
File: holidays/countries/yemen.py:158-163
Timestamp: 2025-05-06T13:01:22.693Z
Learning: In the holidays library, the RAMADAN_BEGINNING_DATES dictionary in country-specific Islamic holiday classes (like YemenIslamicHolidays) is used indirectly through the backend. When a country class calls _add_holiday_29_ramadan(), the IslamicHolidays implementation uses the country's custom calendar dates to calculate the 29th day of Ramadan by adding 28 days to the beginning date.
Applied to files:
holidays/groups/islamic.py
📚 Learning: 2025-08-26T21:29:47.753Z
Learnt from: KJhellico
PR: vacanza/holidays#2860
File: tests/common.py:365-372
Timestamp: 2025-08-26T21:29:47.753Z
Learning: In the holidays library, countries with Islamic holidays inherit directly from the IslamicHolidays class (e.g., `class Afghanistan(HolidayBase, InternationalHolidays, IslamicHolidays)`). The separate `_CustomIslamicHolidays` classes (like `AfghanistanIslamicHolidays`) are helper classes for specific date data, not the main country classes. Therefore, `isinstance(holidays_instance, IslamicHolidays)` is sufficient to detect all countries with Islamic holidays.
Applied to files:
holidays/groups/islamic.py
📚 Learning: 2025-08-11T07:56:47.176Z
Learnt from: Wasif-Shahzad
PR: vacanza/holidays#2791
File: holidays/countries/syrian_arab_republic.py:98-104
Timestamp: 2025-08-11T07:56:47.176Z
Learning: In Syria, Arafah Day is considered part of the Eid al-Adha holidays and uses the same name "عيد الأضحى" rather than having a distinct name like "يوم عرفة" as in some other countries.
Applied to files:
holidays/groups/islamic.py
📚 Learning: 2025-08-19T20:36:15.300Z
Learnt from: KJhellico
PR: vacanza/holidays#2831
File: holidays/countries/south_sudan.py:84-88
Timestamp: 2025-08-19T20:36:15.300Z
Learning: In South Sudan holidays implementation, Eid al-Fitr and Eid al-Adha holidays should use the ISLAMIC category system. The first day of each Eid is a national public holiday (appears in both PUBLIC and ISLAMIC categories), while additional days are typically Muslim-specific and should only appear in the ISLAMIC category through the _populate_islamic_holidays() method.
Applied to files:
holidays/groups/islamic.py
📚 Learning: 2025-08-19T20:39:52.024Z
Learnt from: KJhellico
PR: vacanza/holidays#2831
File: holidays/countries/south_sudan.py:84-88
Timestamp: 2025-08-19T20:39:52.024Z
Learning: In South Sudan holidays implementation, Eid al-Fitr and Eid al-Adha follow a specific two-day pattern: the first day of each Eid is a PUBLIC holiday (national holiday for everyone), while the second day is an ISLAMIC category holiday (Muslim-specific only). This should be implemented by keeping first day calls in _populate_public_holidays() and adding second day calls in _populate_islamic_holidays() method.
Applied to files:
holidays/groups/islamic.py
📚 Learning: 2025-04-03T13:03:16.558Z
Learnt from: KJhellico
PR: vacanza/holidays#2398
File: holidays/countries/guinea.py:101-106
Timestamp: 2025-04-03T13:03:16.558Z
Learning: For Islamic holidays in Guinea like "Lendemain de la nuit Lailatoul Qadr" (Day after Night of Power) and "Lendemain de la nuit du Maoloud" (Day after Prophet's Birthday), the naming refers to the daylight hours following the night when these Islamic observances occur. Since in the Islamic calendar days begin at sunset rather than midnight, methods like `_add_laylat_al_qadr_day` and `_add_mawlid_day` correctly calculate these dates without requiring an additional day offset in the implementation.
Applied to files:
holidays/groups/islamic.py
📚 Learning: 2025-03-30T13:54:34.376Z
Learnt from: KJhellico
PR: vacanza/holidays#2388
File: holidays/countries/ivory_coast.py:98-107
Timestamp: 2025-03-30T13:54:34.376Z
Learning: In the Islamic calendar, days begin at sunset rather than midnight. For holidays like "Lendemain de la Nuit du Destin" (Day after Night of Destiny) in Ivory Coast, the name refers to the daylight hours following the night of Laylat al-Qadr (27th of Ramadan). The implementation uses `_add_laylat_al_qadr_day` which correctly calculates this as the 27th day of Ramadan in the Gregorian calendar.
Applied to files:
holidays/groups/islamic.py
📚 Learning: 2025-04-03T13:03:16.558Z
Learnt from: KJhellico
PR: vacanza/holidays#2398
File: holidays/countries/guinea.py:101-106
Timestamp: 2025-04-03T13:03:16.558Z
Learning: For Islamic holidays in Guinea like "Lendemain de la nuit Lailatoul Qadr" (Day after Night of Power) and "Lendemain de la nuit du Maoloud" (Day after Prophet's Birthday), the naming refers to the daylight hours following the night when these Islamic observances occur. Since in the Islamic calendar days begin at sunset rather than midnight, methods like `_add_laylat_al_qadr_day` and `_add_mawlid_day` correctly calculate these dates without requiring an additional day offset in the implementation, following the same pattern as in the Ivory Coast implementation.
Applied to files:
holidays/groups/islamic.py
🧬 Code graph analysis (8)
holidays/countries/taiwan.py (1)
holidays/observed_holiday_base.py (1)
ObservedRule
(20-24)
holidays/groups/hindu.py (2)
holidays/calendars/hindu.py (1)
_HinduLunisolar
(50-1491)holidays/groups/eastern.py (1)
_add_eastern_calendar_holiday_set
(52-73)
holidays/groups/mongolian.py (1)
holidays/calendars/mongolian.py (1)
_MongolianLunisolar
(24-338)
holidays/groups/hebrew.py (3)
holidays/calendars/hebrew.py (1)
_HebrewLunisolar
(30-1633)holidays/holiday_base.py (1)
_add_holiday
(797-809)holidays/calendars/gregorian.py (1)
_timedelta
(37-42)
holidays/groups/sinhala.py (2)
holidays/calendars/sinhala.py (1)
_SinhalaLunar
(35-410)holidays/groups/eastern.py (1)
_add_eastern_calendar_holiday_set
(52-73)
holidays/groups/buddhist.py (1)
holidays/calendars/buddhist.py (1)
_BuddhistLunisolar
(23-440)
holidays/groups/balinese_saka.py (1)
holidays/calendars/balinese_saka.py (1)
_BalineseSakaLunar
(21-112)
holidays/groups/islamic.py (2)
holidays/calendars/islamic.py (2)
_IslamicLunar
(61-4082)imam_mahdi_birthday_dates
(4048-4049)holidays/groups/eastern.py (1)
_add_eastern_calendar_holiday_set
(52-73)
⏰ Context from checks skipped due to timeout of 300000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: Test Python 3.13 on windows-latest
- GitHub Check: Test Python 3.14 on windows-latest
🔇 Additional comments (7)
holidays/countries/taiwan.py (1)
115-116
: Signature formatting is fine.Single-line signature keeps the exact parameters and defaults, so behavior stays intact. Nicely aligned with the parent class.
holidays/groups/eastern.py (1)
52-73
: Good: concise batch helper mirrors singleton logicSet-comprehension + delegation keeps behavior consistent and dedupes dates.
holidays/groups/sinhala.py (1)
61-63
: Nice delegation to shared batch helperKeeps logic centralized and consistent with other groups.
holidays/groups/tibetan.py (2)
16-16
: Import path update looks rightAligns with calendars module structure.
106-112
: Return type change approvedThe helper
_add_losar_day_two
is only invoked inholidays/countries/bhutan.py
, and its return value is discarded there. No other callers or type hints expect aset[date]
. The change toOptional[date]
is safe.holidays/groups/islamic.py (2)
38-41
: Refactor to centralized batch helper looks solidUnified pattern reduces duplication and standardizes estimated-label handling. No behavior regressions apparent.
Also applies to: 48-51, 58-61, 68-71, 81-84, 93-96, 105-108, 118-121, 128-131, 138-141, 148-151, 161-164, 171-174, 181-184, 191-194, 201-204, 211-214, 221-224, 231-234, 241-244, 251-254, 261-264, 271-274, 284-287, 298-301, 309-312, 320-323, 330-333, 342-345, 354-357, 367-370, 384-387, 394-397, 404-407, 416-419, 426-429, 436-439, 446-449
275-286
: Helper naming and delegation are clear
_add_islamic_calendar_holiday_set
thinly wraps the eastern batch helper with the module’sshow_estimated
flag. Good separation.
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks solid 👍
LGTM, just waiting for @PPsyrius' review
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM 🛠️
Proposed change
Refactor Eastern holidays groups.
Type of change
holidays
functionality in general)Checklist
make check
locally; all checks and tests passed.