+
Skip to content

Conversation

KJhellico
Copy link
Collaborator

Proposed change

Refactor Eastern holidays groups.

Type of change

  • New country/market holidays support (thank you!)
  • Supported country/market holidays update (calendar discrepancy fix, localization)
  • Existing code/documentation/test/process quality improvement (best practice, cleanup, refactoring, optimization)
  • Dependency update (version deprecation/pin/upgrade)
  • Bugfix (non-breaking change which fixes an issue)
  • Breaking change (a code change causing existing functionality to break)
  • New feature (new holidays functionality in general)

Checklist

Copy link
Contributor

coderabbitai bot commented Sep 28, 2025

Summary by CodeRabbit

  • Refactor

    • Standardized holiday-calculation helpers across Eastern, Islamic, Hindu, Sinhala, and Hebrew groups for batch additions and consistency.
    • Simplified Tibetan Losar Day Two handling.
  • Bug Fixes

    • Hanukkah dates now return only valid dates, improving reliability in edge cases.
  • Chores

    • Updated imports to new calendar module locations across multiple groups.
    • Adjusted tooling configuration to preserve runtime typing in linting/upgrade processes.
  • Style

    • Minor signature formatting cleanup in Taiwan observed-holiday logic (no behavior change).

Walkthrough

Refactors 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

Cohort / File(s) Summary
Signature formatting
holidays/countries/taiwan.py
Reformatted _populate_observed definition to a single-line parameter list; removed an inline comment and noqa:UP045. No logic changes.
Calendar import path updates
holidays/groups/balinese_saka.py, holidays/groups/buddhist.py, holidays/groups/mongolian.py, holidays/groups/islamic.py, holidays/groups/sinhala.py, holidays/groups/hebrew.py, holidays/groups/tibetan.py
Changed imports to module-specific subpaths (e.g., holidays.calendars.*) for respective lunar/lunisolar calendars. Usage unchanged.
Eastern helper addition
holidays/groups/eastern.py
Added _add_eastern_calendar_holiday_set(name, dts_estimated, show_estimated=True, days_delta=0) -> set[date] to batch-add holidays and return the set of added dates.
Delegation to Eastern helper
holidays/groups/islamic.py, holidays/groups/sinhala.py, holidays/groups/hindu.py
Replaced local loop-based helpers with delegation to _add_eastern_calendar_holiday_set. In islamic.py renamed helper to _add_islamic_calendar_holiday_set and updated call sites; parameter datesdts_estimated.
Hebrew helper refactor
holidays/groups/hebrew.py, holidays/calendars/hebrew.py
Rewrote accumulation using comprehensions to filter out None and return set[date] for hanukkah-related methods; updated _HebrewLunisolar.hanukkah_date signature to set[date].
Tibetan return type change
holidays/groups/tibetan.py
_add_losar_day_two simplified to return Optional[date] (was set[date]) by delegating to _add_tibetan_calendar_holiday(..., days_delta=+1).
Tooling configuration
pyproject.toml
Added lint.pyupgrade.keep-runtime-typing = true.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~25 minutes

Possibly related PRs

Suggested reviewers

  • arkid15r
  • PPsyrius

Pre-merge checks and finishing touches

✅ Passed checks (3 passed)
Check name Status Explanation
Title Check ✅ Passed The title “Refactor Eastern holidays groups” succinctly captures the primary intent of the pull request by describing the refactoring work performed on the Eastern calendar group modules, as evidenced by the import path updates and new helper methods across those files. It is concise, clear, and focused on the main change without unnecessary detail or noise, enabling a reviewer to quickly grasp the PR’s purpose. This aligns directly with the PR objectives and the detailed changes presented in the raw summary.
Description Check ✅ Passed The pull request description directly states that the change involves refactoring the Eastern holidays groups and includes relevant context—such as the type of change and confirmation that contributing guidelines were followed and tests passed—which matches the actual modifications. It remains on-topic by summarizing the PR’s purpose and ties back to the detailed file changes. Though brief, it clearly relates to the changeset and provides sufficient context for this check.
Docstring Coverage ✅ Passed No functions found in the changes. Docstring coverage check skipped.
✨ Finishing touches
  • 📝 Generate Docstrings
🧪 Generate unit tests
  • Create PR with unit tests
  • Post copyable unit tests in a comment

📜 Recent review details

Configuration used: Path: .coderabbit.yaml

Review profile: ASSERTIVE

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 02175e1 and 50a5ee1.

📒 Files selected for processing (2)
  • holidays/calendars/hebrew.py (1 hunks)
  • holidays/groups/hebrew.py (3 hunks)
🧰 Additional context used
🧠 Learnings (3)
📓 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-23T09:59:19.886Z
Learnt from: PPsyrius
PR: vacanza/holidays#2490
File: holidays/countries/ethiopia.py:45-45
Timestamp: 2025-04-23T09:59:19.886Z
Learning: For the Ethiopia holidays class, it's appropriate to add a return type hint only to the `_is_leap_year` method to match the base class implementation in `holidays/holiday_base.py`, while keeping other methods without type hints to maintain consistency with other country implementations.

Applied to files:

  • holidays/groups/hebrew.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
🧬 Code graph analysis (2)
holidays/groups/hebrew.py (3)
holidays/calendars/gregorian.py (1)
  • _timedelta (37-42)
holidays/calendars/hebrew.py (2)
  • _HebrewLunisolar (30-1633)
  • hanukkah_date (1605-1606)
holidays/holiday_base.py (1)
  • _add_holiday (797-809)
holidays/calendars/hebrew.py (2)
holidays/calendars/chinese.py (1)
  • _get_holiday (1343-1360)
holidays/calendars/islamic.py (1)
  • _get_holiday (3973-3990)
⏰ Context from checks skipped due to timeout of 300000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
  • GitHub Check: Test Python 3.13 on windows-latest
🔇 Additional comments (1)
holidays/groups/hebrew.py (1)

29-39: Let's keep guarding holiday_date before _timedelta fires
Line 30 still accepts the Optional results we hand down (see the lingering # type: ignore[arg-type] sites). For years outside the curated tables, lag_baomer_date et al. return None, so _timedelta(holiday_date, delta) will raise at runtime. A tiny guard keeps the new comprehension tidy while preserving safety.

-from typing import Union
+from typing import Optional, Union
@@
-    def _add_hebrew_calendar_holiday(
-        self, name: str, holiday_date: date, days_delta: Union[int, Iterable[int]] = 0
-    ) -> set[date]:
+    def _add_hebrew_calendar_holiday(
+        self, name: str, holiday_date: Optional[date], days_delta: Union[int, Iterable[int]] = 0
+    ) -> set[date]:
         """
         Add Hebrew calendar holiday.
         """
-        return {
-            dt
-            for delta in ((days_delta,) if isinstance(days_delta, int) else days_delta)
-            if (dt := self._add_holiday(name, _timedelta(holiday_date, delta)))
-        }
+        if holiday_date is None:
+            return set()
+        deltas = (days_delta,) if isinstance(days_delta, int) else days_delta
+        return {
+            dt
+            for delta in deltas
+            if (dt := self._add_holiday(name, _timedelta(holiday_date, delta)))
+        }

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
�[1mCause:�[0m Failed to load configuration /ruff.toml
�[1mCause:�[0m Failed to parse /ruff.toml
�[1mCause:�[0m TOML parse error at line 26, column 3
|
26 | "RSE100", # Use of assert detected
| ^^^^^^^^
Unknown rule selector: RSE100

holidays/groups/hebrew.py

�[1;31mruff failed�[0m
�[1mCause:�[0m Failed to load configuration /ruff.toml
�[1mCause:�[0m Failed to parse /ruff.toml
�[1mCause:�[0m TOML parse error at line 26, column 3
|
26 | "RSE100", # Use of assert detected
| ^^^^^^^^
Unknown rule selector: RSE100


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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

Copy link

codecov bot commented Sep 28, 2025

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 100.00%. Comparing base (f322583) to head (50a5ee1).
⚠️ Report is 3 commits behind head on dev.

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.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

Copy link
Contributor

@coderabbitai coderabbitai bot left a 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 Optional

Once _add_hebrew_calendar_holiday takes Optional[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() yields set[Optional[date]]. Returning that leaks Nones. Consider filtering to set[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

📥 Commits

Reviewing files that changed from the base of the PR and between f322583 and 02175e1.

📒 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 logic

Set-comprehension + delegation keeps behavior consistent and dedupes dates.

holidays/groups/sinhala.py (1)

61-63: Nice delegation to shared batch helper

Keeps logic centralized and consistent with other groups.

holidays/groups/tibetan.py (2)

16-16: Import path update looks right

Aligns with calendars module structure.


106-112: Return type change approved

The helper _add_losar_day_two is only invoked in holidays/countries/bhutan.py, and its return value is discarded there. No other callers or type hints expect a set[date]. The change to Optional[date] is safe.

holidays/groups/islamic.py (2)

38-41: Refactor to centralized batch helper looks solid

Unified 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’s show_estimated flag. Good separation.

Copy link

Copy link
Collaborator

@arkid15r arkid15r left a 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

Copy link
Collaborator

@PPsyrius PPsyrius left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM 🛠️

@arkid15r arkid15r added this pull request to the merge queue Sep 29, 2025
Merged via the queue into vacanza:dev with commit c68e6d6 Sep 29, 2025
36 checks passed
@KJhellico KJhellico deleted the ref-eastern branch September 29, 2025 15:54
@coderabbitai coderabbitai bot mentioned this pull request Oct 3, 2025
9 tasks
@arkid15r arkid15r mentioned this pull request Oct 6, 2025
@coderabbitai coderabbitai bot mentioned this pull request Oct 9, 2025
9 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants

点击 这是indexloc提供的php浏览器服务,不要输入任何密码和下载