+
Skip to content

Conversation

KJhellico
Copy link
Collaborator

Proposed change

Update Saudi Arabia holidays:

  • update Eid al-Fitr celebration rule
  • update tests

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 Jul 25, 2025

Summary by CodeRabbit

  • New Features

    • Improved handling and calculation of Saudi Arabian public holidays, including more accurate determination of Eid al-Fitr and Eid al-Adha dates.
    • Enhanced localization for holiday names in both Arabic and English, reflecting updated and corrected translations.
  • Bug Fixes

    • Corrected logic for observed holidays, ensuring observed days are properly added and named.
  • Tests

    • Expanded and reorganized test coverage for Saudi Arabian holidays, with focused tests for each major holiday and improved localization checks.
    • Removed outdated tests related to weekends and Hijri calendar overlaps.
  • Documentation

    • Updated and clarified holiday descriptions and comments in localization files for better consistency and accuracy.

Walkthrough

This update refactors Saudi Arabia holiday calculations, introduces a custom Islamic holidays subclass, and adjusts logic for observed and Islamic-based holidays. Localization files for Arabic and English are updated for consistency and accuracy. The Saudi Arabia test suite is reorganized and expanded, with focused tests and improved localization checks.

Changes

Files/Paths Change Summary
holidays/countries/saudi_arabia.py Refactored imports, logic for observed/Islamic holidays, added SaudiArabiaIslamicHolidays subclass, updated method signatures, docs.
holidays/groups/islamic.py Removed _add_eid_al_fitr_day_four method.
holidays/calendars/islamic.py Added _is_long_ramadan method to _IslamicLunar class.
holidays/locale/ar/LC_MESSAGES/SA.po, holidays/locale/en_US/LC_MESSAGES/SA.po Updated comments, swapped/corrected msgid strings for National/Founding Day, reordered entries, punctuation fixes.
tests/countries/test_saudi_arabia.py Reorganized and expanded tests, removed weekend/Hijri overlap tests, improved localization checks, updated method signatures.

Estimated code review effort

🎯 4 (Complex) | ⏱️ ~40 minutes

Suggested reviewers

  • KJhellico

Note

⚡️ Unit Test Generation is now available in beta!

Learn more here, or try it out under "Finishing Touches" below.


📜 Recent review details

Configuration used: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 8f83803 and f71e42d.

📒 Files selected for processing (1)
  • holidays/calendars/islamic.py (2 hunks)
🧰 Additional context used
🧠 Learnings (2)
📓 Common learnings
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.
Learnt from: KJhellico
PR: vacanza/holidays#2623
File: README.md:0-0
Timestamp: 2025-07-10T11:00:13.195Z
Learning: The COUNTRIES dictionary in holidays/registry.py contains 211 entries as confirmed by KJhellico in PR #2623, not 214 as previously calculated.
Learnt from: KJhellico
PR: vacanza/holidays#2720
File: README.md:108-110
Timestamp: 2025-07-17T11:07:04.986Z
Learning: Always verify country counts in the holidays library by checking the current count in the COUNTRIES dictionary in holidays/registry.py directly, rather than relying on information from previous PRs, as the count changes frequently with new country additions.
Learnt from: KJhellico
PR: vacanza/holidays#2631
File: tests/countries/test_sint_maarten.py:62-0
Timestamp: 2025-06-14T21:12:07.224Z
Learning: KJhellico prefers to focus on completing and reviewing the main holiday implementation code before doing detailed reviews of the corresponding test files.
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.
Learnt from: KJhellico
PR: vacanza/holidays#2719
File: README.md:108-108
Timestamp: 2025-07-12T20:54:28.749Z
Learning: In PR #2719 adding Faroe Islands, KJhellico confirmed that the country count in README.md should be updated from 213 to 214, making 214 the accurate total after this addition.
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.
Learnt from: KJhellico
PR: vacanza/holidays#2354
File: holidays/countries/fiji.py:171-183
Timestamp: 2025-03-29T15:15:05.919Z
Learning: In the Fiji holidays implementation, the maintainers are aware of the need to extend the MAWLID_DATES dictionary beyond 2025 when future official references become available, and will do so when appropriate. No suggestions about extending this dictionary should be made in future reviews.
Learnt from: PPsyrius
PR: vacanza/holidays#2537
File: tests/countries/test_finland.py:23-26
Timestamp: 2025-05-09T18:36:09.607Z
Learning: The holidays project prioritizes complete historical coverage in tests, verifying holidays from their first year of observance (e.g., 1853 for Finland) through future projections, rather than using shorter sliding windows.
Learnt from: KJhellico
PR: vacanza/holidays#2614
File: README.md:108-108
Timestamp: 2025-06-14T20:13:29.536Z
Learning: In the vacanza/holidays repository, the team prefers to manually update the country count in README.md when adding new countries, with tests in place to verify the count stays accurate. They are satisfied with this approach and do not want it automated.
Learnt from: PPsyrius
PR: vacanza/holidays#2388
File: holidays/countries/ivory_coast.py:84-85
Timestamp: 2025-03-30T08:08:30.415Z
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.
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.
Learnt from: KJhellico
PR: vacanza/holidays#2530
File: tests/countries/test_andorra.py:23-28
Timestamp: 2025-05-06T21:07:11.577Z
Learning: In the holidays project, test methods for country holidays follow a standard form where year ranges are explicitly recreated in each test method rather than being stored as class variables, to maintain consistency across all country tests.
Learnt from: KJhellico
PR: vacanza/holidays#2398
File: holidays/countries/guinea.py:106-110
Timestamp: 2025-04-04T10:52:41.546Z
Learning: In the Guinea holidays implementation, observed Eid al-Fitr cases are properly covered by the test_eid_al_fitr_day() method, which tests both the regular holiday dates and the observed dates when the holiday falls on a non-working day (for years >= 2023).
Learnt from: PPsyrius
PR: vacanza/holidays#2643
File: holidays/countries/mauritius.py:144-169
Timestamp: 2025-06-19T02:34:18.382Z
Learning: Custom holiday classes that extend _CustomHinduHolidays, _CustomIslamicHolidays, _CustomBuddhistHolidays, etc. in the holidays library do not use docstrings. They follow a pattern of using only inline comments above date dictionaries, as seen in Malaysia, Singapore, UAE, and other country implementations.
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.
Learnt from: KJhellico
PR: vacanza/holidays#2653
File: holidays/locale/th/LC_MESSAGES/TW.po:17-21
Timestamp: 2025-06-21T18:06:50.027Z
Learning: KJhellico's username includes a tilde character (~) as part of their nickname (appears as "~Jhellico" in Last-Translator headers), which is intentional formatting and not an error.
holidays/calendars/islamic.py (17)

Learnt from: Wasif-Shahzad
PR: #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.

Learnt from: Wasif-Shahzad
PR: #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.

Learnt from: Wasif-Shahzad
PR: #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: KJhellico
PR: #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.

Learnt from: KJhellico
PR: #2398
File: holidays/countries/guinea.py:106-110
Timestamp: 2025-04-04T10:52:41.546Z
Learning: In the Guinea holidays implementation, observed Eid al-Fitr cases are covered by the test_eid_al_fitr_day() method, which tests both regular holiday dates and the observed dates when the holiday falls on a non-working day (for years >= 2023).

Learnt from: KJhellico
PR: #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.

Learnt from: KJhellico
PR: #2398
File: holidays/countries/guinea.py:106-110
Timestamp: 2025-04-04T10:52:41.546Z
Learning: In the Guinea holidays implementation, observed Eid al-Fitr cases are properly covered by the test_eid_al_fitr_day() method, which tests both the regular holiday dates and the observed dates when the holiday falls on a non-working day (for years >= 2023).

Learnt from: KJhellico
PR: #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.

Learnt from: KJhellico
PR: #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.

Learnt from: PPsyrius
PR: #2388
File: holidays/countries/ivory_coast.py:84-85
Timestamp: 2025-03-30T08:08:30.415Z
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.

Learnt from: Wasif-Shahzad
PR: #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 like FEB, TUE, MAR, and SUN internally through parent class implementations even when these constants don't appear directly in the file.

Learnt from: KJhellico
PR: #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.

Learnt from: KJhellico
PR: #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.

Learnt from: KJhellico
PR: #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.

Learnt from: PPsyrius
PR: #2643
File: holidays/countries/mauritius.py:171-184
Timestamp: 2025-06-19T02:34:14.456Z
Learning: In the holidays library, _CustomIslamicHolidays subclasses follow a consistent pattern of NOT having docstrings. They go directly to defining date dictionaries, as evidenced by Malaysia, Singapore, UAE, and dozens of other country implementations.

Learnt from: PPsyrius
PR: #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.

Learnt from: PPsyrius
PR: #2643
File: holidays/countries/mauritius.py:144-169
Timestamp: 2025-06-19T02:34:18.382Z
Learning: Custom holiday classes that extend _CustomHinduHolidays, _CustomIslamicHolidays, _CustomBuddhistHolidays, etc. in the holidays library do not use docstrings. They follow a pattern of using only inline comments above date dictionaries, as seen in Malaysia, Singapore, UAE, and other country implementations.

⏰ Context from checks skipped due to timeout of 300000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
  • GitHub Check: Test build on windows-latest
  • GitHub Check: Test build on macos-latest
  • GitHub Check: Test build on ubuntu-latest
🔇 Additional comments (2)
holidays/calendars/islamic.py (2)

17-31: Good addition of the required import.

The _timedelta import is properly placed and necessary for the new _is_long_ramadan method's date arithmetic functionality.


3980-3995: Well-implemented Ramadan duration check.

The method correctly determines if Ramadan lasted 30 days by checking if the date 30 days before Eid al-Fitr matches any Ramadan beginning date. The logic is sound, the implementation is efficient with set comprehension, and the docstring is properly formatted.

✨ Finishing Touches
  • 📝 Generate Docstrings
🧪 Generate unit tests
  • Create PR with unit tests
  • Post copyable unit tests in a comment

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
🪧 Tips

Chat

There are 3 ways to chat with CodeRabbit:

  • Review comments: Directly reply to a review comment made by CodeRabbit. Example:
    • I pushed a fix in commit <commit_id>, please review it.
    • Explain this complex logic.
    • Open a follow-up GitHub issue for this discussion.
  • Files and specific lines of code (under the "Files changed" tab): Tag @coderabbitai in a new review comment at the desired location with your query. Examples:
    • @coderabbitai explain this code block.
    • @coderabbitai modularize this function.
  • PR comments: Tag @coderabbitai in a new PR comment to ask questions about the PR branch. For the best results, please provide a very specific query, as very limited context is provided in this mode. Examples:
    • @coderabbitai gather interesting stats about this repository and render them as a table. Additionally, render a pie chart showing the language distribution in the codebase.
    • @coderabbitai read src/utils.ts and explain its main purpose.
    • @coderabbitai read the files in the src/scheduler package and generate a class diagram using mermaid and a README in the markdown format.
    • @coderabbitai help me debug CodeRabbit configuration file.

Support

Need 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)

  • @coderabbitai pause to pause the reviews on a PR.
  • @coderabbitai resume to resume the paused reviews.
  • @coderabbitai review to trigger an incremental review. This is useful when automatic reviews are disabled for the repository.
  • @coderabbitai full review to do a full review from scratch and review all the files again.
  • @coderabbitai summary to regenerate the summary of the PR.
  • @coderabbitai generate docstrings to generate docstrings for this PR.
  • @coderabbitai generate sequence diagram to generate a sequence diagram of the changes in this PR.
  • @coderabbitai generate unit tests to generate unit tests for this PR.
  • @coderabbitai resolve resolve all the CodeRabbit review comments.
  • @coderabbitai configuration to show the current CodeRabbit configuration for the repository.
  • @coderabbitai help to get help.

Other keywords and placeholders

  • Add @coderabbitai ignore anywhere in the PR description to prevent this PR from being reviewed.
  • Add @coderabbitai summary to generate the high-level summary at a specific location in the PR description.
  • Add @coderabbitai anywhere in the PR title to generate the title automatically.

Documentation and Community

  • Visit our Documentation for detailed information on how to use CodeRabbit.
  • Join our Discord Community to get help, request features, and share feedback.
  • Follow us on X/Twitter for updates and announcements.

Copy link

codecov bot commented Jul 25, 2025

Codecov Report

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

Additional details and impacted files
@@            Coverage Diff            @@
##               dev     #2753   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files          271       271           
  Lines        16205     16208    +3     
  Branches      2190      2190           
=========================================
+ Hits         16205     16208    +3     

☔ 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.

PPsyrius
PPsyrius previously approved these changes Jul 25, 2025
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 🇸🇦

Copy link

@arkid15r arkid15r added this pull request to the merge queue Jul 26, 2025
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.

Great refactoring 👍

Merged via the queue into vacanza:dev with commit d486d90 Jul 26, 2025
36 checks passed
@KJhellico KJhellico deleted the upd-saudi-arabia branch July 26, 2025 13:21
@KJhellico KJhellico mentioned this pull request Aug 4, 2025
@coderabbitai coderabbitai bot mentioned this pull request Sep 10, 2025
9 tasks
@coderabbitai coderabbitai bot mentioned this pull request Sep 28, 2025
9 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants

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