Add Oman holidays #2463
Add Oman holidays #2463
Conversation
Summary by CodeRabbit
Summary by CodeRabbit
WalkthroughThis change introduces comprehensive support for Oman’s public holidays. It adds a new module for Oman, updates the registry and documentation, provides localization files in Arabic and English, and implements a suite of unit tests. The Oman holiday calendar includes both fixed-date and Islamic holidays, with logic for weekend changes and multi-day events. The code also introduces new helper methods for Islamic holidays and ensures that Oman’s holidays are accessible via the main countries package. Changes
Assessment against linked issues
Suggested reviewers
Tip ⚡💬 Agentic Chat (Pro Plan, General Availability)
📜 Recent review detailsConfiguration used: .coderabbit.yaml 📒 Files selected for processing (3)
🧰 Additional context used🧬 Code Graph Analysis (1)holidays/countries/__init__.py (1)
🔇 Additional comments (4)
✨ Finishing Touches
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
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
|
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## dev #2463 +/- ##
=========================================
Coverage 100.00% 100.00%
=========================================
Files 212 213 +1
Lines 13454 13510 +56
Branches 1922 1928 +6
=========================================
+ Hits 13454 13510 +56 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Signed-off-by: Wasif Shahzad <159248937+Wasif-Shahzad@users.noreply.github.com>
Signed-off-by: Wasif Shahzad <159248937+Wasif-Shahzad@users.noreply.github.com>
Signed-off-by: Wasif Shahzad <159248937+Wasif-Shahzad@users.noreply.github.com>
Signed-off-by: Wasif Shahzad <159248937+Wasif-Shahzad@users.noreply.github.com>
There was a problem hiding this comment.
Actionable comments posted: 2
🔭 Outside diff range comments (2)
holidays/locale/ar/LC_MESSAGES/OM.po (1)
35-65: 🧹 Nitpick (assertive)Consider providing translated strings for these Arabic msgids.
All msgstr entries for Sultan's Accession Day, Renaissance Day, National Day, and other holidays are empty. If these messages are intended for Arabic speakers, please provide accurate Arabic translations or confirm that the empty strings are intentional placeholders.
holidays/countries/oman.py (1)
125-187: 🧹 Nitpick (assertive)Keep the stored dates updated.
The holiday date dictionaries reference timeanddate.com up to 2025. Consider verifying new official decrees or changes for Omani Islamic holidays beyond that range if this library is meant to remain up-to-date.
Do you want me to open a follow-up issue to widen date coverage based on evolving official calendars?
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (5)
holidays/countries/oman.py(1 hunks)holidays/groups/islamic.py(1 hunks)holidays/locale/ar/LC_MESSAGES/OM.po(1 hunks)holidays/locale/en_US/LC_MESSAGES/OM.po(1 hunks)tests/countries/test_oman.py(1 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (2)
holidays/groups/islamic.py (1)
holidays/calendars/islamic.py (1)
ramadan_beginning_dates(3868-3869)
holidays/countries/oman.py (4)
holidays/calendars/islamic.py (1)
_CustomIslamicHolidays(3881-3882)holidays/calendars/gregorian.py (1)
_timedelta(37-42)holidays/groups/islamic.py (2)
IslamicHolidays(20-426)_add_islamic_new_year_day(283-295)holidays/holiday_base.py (1)
HolidayBase(57-1275)
🔇 Additional comments (15)
holidays/groups/islamic.py (1)
245-253: Confirm 29th day of Ramadan offset is correct.Adding 28 days to the Ramadan start date to determine the 29th day appears logical. However, confirm whether any Oman-specific decrees shift certain holidays if the 29th coincides with public weekends or other holidays.
holidays/countries/oman.py (3)
56-67: Constructor structure looks fine.The choice to delegate Islamic holiday logic via
OmanIslamicHolidaysis straightforward. Ensure thatislamic_show_estimatedis tested to confirm it toggles the estimated label as expected.
68-108: Weekend logic and decrees appear consistent.The code respects the shift from Thursday-Friday to Friday-Saturday on May 1, 2013, and references relevant decrees for Oman. Good job ensuring distinct holiday sets before and after 2020.
104-108: Potential overlap with Ramadan-based holidays.You are adding Eid al-Fitr Eve if the 29th of Ramadan does not conflict with existing days. This could result in skipping Eid Eve if some other code block has already populated that date. Confirm whether that scenario aligns with official decrees about consecutive holidays.
tests/countries/test_oman.py (11)
1-17: Header and imports look good.The file header properly includes copyright information and follows the project's standard format. Imports are clean and appropriate for the test case.
19-25: Test class setup is well-structured.The setup establishes a good test range (1970-2050) and includes a specialized instance for testing without estimated Islamic holidays. This approach is thorough and follows best practices for holiday testing.
26-28: Country alias test is correct.Good validation of Oman's ISO code aliases (OM, OMN).
29-31: No holidays test is appropriate.Testing that no holidays exist before the expected range (1969) is a good defensive check.
32-41: Fixed date holiday tests for Accession Day and Renaissance Day look correct.The tests accurately reflect Oman's political timeline:
- Accession Day starting from 2020
- Renaissance Day from 1970-2020 (correctly ending in 2020)
This demonstrates good attention to the historical accuracy of Oman's holidays.
42-49: National Day test implementation is appropriate.The test correctly handles:
- National Day celebration from 2020 onwards
- Two-day celebration pattern (18-19 Nov and later 20-21 Nov)
- Transition to new dates from 2025
Good handling of the complex date pattern.
50-63: Islamic New Year test is comprehensive.The test includes specific dates for 2018-2024 and verifies behavior with both estimated and non-estimated holiday instances. This dual testing approach ensures the Islamic calendar conversion logic works correctly.
64-92: Mawlid and Isra and Miraj tests are thorough.Both tests:
- Include specific dates for multiple years
- Verify behavior with the no_estimated_holidays instance
- Use the correct Arabic names for the holidays
This demonstrates careful attention to Islamic calendar conversions.
93-158: Eid holiday tests are extensive and accurate.The tests for Eid al-Fitr and Eid al-Adha:
- Include multi-day holiday periods (appropriate for these major festivals)
- Cover numerous years (2019-2025)
- Test both estimated and non-estimated scenarios
The multi-day nature of these holidays is correctly represented.
160-195: Year-specific tests provide good validation.The tests for 2019 and 2021 verify the complete holiday sets for these years, including:
- The transition from Renaissance Day to Accession Day
- Introduction of National Day in 2021
- Proper handling of estimated holidays (marked with مُقدَّر)
This approach helps validate the overall holiday calculation logic.
196-234: Localization tests are comprehensive.The tests verify both:
- Default locale (Arabic)
- English (US) translations
All holiday names are correctly translated between Arabic and English, maintaining proper diacritical marks and apostrophes where needed (e.g., "Isra' and Mi'raj").
Signed-off-by: Wasif Shahzad <159248937+Wasif-Shahzad@users.noreply.github.com>
|
Explanation about the changes I made Oman had Eid al-Fitr holidays spanning from 29th of Ramadan (9th month of Islamic calendar) to 3rd of Shawwal (basically For Eid al-Adha, there were also 4 holidays but it was much simpler. I only needed to use In the Decree 88/2022, they say that holidays are compensated in case if a public holiday falls on weekend but there are no official rulings on what happens. Sometimes they give holiday, sometimes they give payment as a compensation so I left it as per the suggestion of @KJhellico . All of these changes were made with the help of @KJhellico |
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (2)
holidays/countries/oman.py(1 hunks)tests/countries/test_oman.py(1 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (2)
tests/countries/test_oman.py (2)
tests/common.py (8)
TestCase(28-338)CommonCountryTests(356-374)assertAliases(121-130)assertNoHolidays(292-294)assertHolidayName(195-199)assertNoHolidayName(273-275)assertHolidays(228-230)assertLocalizedHolidays(327-338)holidays/countries/oman.py (3)
Oman(37-112)OM(115-116)OMN(119-120)
holidays/countries/oman.py (4)
holidays/calendars/islamic.py (1)
_CustomIslamicHolidays(3881-3882)holidays/calendars/gregorian.py (1)
_timedelta(37-42)holidays/groups/islamic.py (13)
IslamicHolidays(20-426)_add_islamic_new_year_day(283-295)_add_mawlid_day(329-339)_add_isra_and_miraj_day(297-306)_add_eid_al_fitr_day(152-163)_add_eid_al_fitr_day_two(165-173)_add_eid_al_fitr_day_three(175-183)_add_holiday_29_ramadan(245-253)_add_eid_al_fitr_eve(195-203)_add_arafah_day(72-83)_add_eid_al_adha_day(109-120)_add_eid_al_adha_day_two(122-130)_add_eid_al_adha_day_three(132-140)holidays/holiday_base.py (1)
HolidayBase(57-1275)
🔇 Additional comments (30)
tests/countries/test_oman.py (16)
1-17: LGTM: Header and imports set up correctly.Standard license header with imports for TestCase, Oman class, and the CommonCountryTests base class needed for holiday testing.
19-25: Good test setup with appropriate year range.The test class extends CommonCountryTests and TestCase correctly. The year range (1970-2050) is comprehensive and matches Oman's start_year. Good addition of the no_estimated_holidays instance for testing both scenarios.
26-28: LGTM: Aliases test looks good.Correctly verifies that OM and OMN are valid aliases for the Oman class.
29-31: LGTM: No holidays validation is correct.Properly verifies that no holidays exist before the start_year (1970).
32-36: LGTM: Accession Day test is accurate.Correctly tests Sultan's Accession Day presence from 2020 onwards and absence before 2020.
37-41: LGTM: Renaissance Day test is accurate.Correctly verifies Renaissance Day from 1970-2019 and its absence from 2020 onwards.
42-49: LGTM: National Day test handles date changes correctly.Properly tests National Day on November 18-19 from 2020-2024 and the transition to November 20-21 from 2025 onwards, as per Decree 15/2025.
50-63: LGTM: Islamic New Year test is comprehensive.Correctly validates specific dates for recent years and confirms the holiday exists across all years, with and without estimated labeling.
64-77: LGTM: Prophet's Birthday (Mawlid) test is correct.Properly tests specific dates for recent years and confirms the holiday exists across all years, with and without estimated labeling.
78-92: LGTM: Isra and Mi'raj test is thorough.Good coverage of specific dates from 2018-2025 and verification across all years with both estimated and non-estimated variants.
93-132: LGTM: Eid al-Fitr test is comprehensive with appropriate date ranges.Test correctly verifies the multiple days of Eid al-Fitr holidays, including the variable number of days across different years (accounting for 29th of Ramadan when applicable).
133-163: LGTM: Eid al-Adha test properly covers all four days of the holiday.Test correctly verifies all four days of Eid al-Adha for each tested year, as implemented in the Oman class.
164-180: LGTM: 2019 holiday test is thorough.Good verification of the complete set of 2019 holidays, including proper labeling of estimated holidays.
181-200: LGTM: 2021 holiday test is comprehensive.Thorough verification of all 2021 holidays, including the new Sultan's Accession Day and National Day holidays that replaced Renaissance Day.
201-219: LGTM: Default localization test is correct.Properly verifies that holidays display in Arabic by default, which is the official language of Oman.
220-238: LGTM: English localization test is thorough.Good verification of English translations for all holidays, confirming proper localization support.
holidays/countries/oman.py (14)
1-12: LGTM: Standard license header.Includes appropriate license and attribution information.
13-35: LGTM: Imports are comprehensive and well-organized.All necessary imports are included with appropriate organization - gettext for translations, calendar components, and holiday base classes.
37-47: LGTM: Class definition with good documentation.The class is properly defined with inheritance from HolidayBase and IslamicHolidays. Documentation includes relevant references to support the implementation.
48-54: LGTM: Country attributes defined correctly.Appropriate country code, language settings, estimated label format, and supported languages are defined properly.
55-66: LGTM: Initialization method handles Islamic holiday options correctly.The constructor properly initializes the IslamicHolidays parent class with the Oman-specific calendar and estimation options.
67-70: LGTM: Weekend handling implements correct policy change.Correctly implements Oman's weekend change from Thursday-Friday to Friday-Saturday starting in 2013.
71-74: LGTM: Sultan's Accession Day implementation is correct.Properly adds the Sultan's Accession Day holiday starting from 2020.
75-78: LGTM: Renaissance Day implementation is correct.Correctly adds Renaissance Day holiday for years up to 2019 only.
79-88: LGTM: National Day implementation handles date changes correctly.Properly implements National Day on November 18-19 for 2020-2024 and changes to November 20-21 from 2025 onwards, as per Decree 15/2025.
89-97: LGTM: Standard Islamic holidays implemented correctly.Islamic New Year, Prophet's Birthday, and Isra' and Mi'raj holidays are correctly implemented using standard helper methods.
98-106: LGTM: Eid al-Fitr implementation handles variable Ramadan length.The implementation correctly adds the standard three days of Eid al-Fitr and handles the 29th of Ramadan appropriately as specified in the requirements.
107-113: LGTM: Eid al-Adha implementation includes all four days.Properly implements all four days of Eid al-Adha: Arafah Day and three Eid days.
115-121: LGTM: Country code aliases defined correctly.OM and OMN are defined as proper aliases for the Oman class.
123-185: LGTM: Custom Islamic holiday dates are well documented.OmanIslamicHolidays class provides specific dates for Islamic holidays with appropriate references to timeanddate.com for verification.
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (1)
holidays/countries/oman.py(1 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (1)
holidays/countries/oman.py (4)
holidays/calendars/islamic.py (1)
_CustomIslamicHolidays(3881-3882)holidays/calendars/gregorian.py (1)
_timedelta(37-42)holidays/groups/islamic.py (13)
IslamicHolidays(20-426)_add_islamic_new_year_day(283-295)_add_mawlid_day(329-339)_add_isra_and_miraj_day(297-306)_add_eid_al_fitr_day(152-163)_add_eid_al_fitr_day_two(165-173)_add_eid_al_fitr_day_three(175-183)_add_holiday_29_ramadan(245-253)_add_eid_al_fitr_eve(195-203)_add_arafah_day(72-83)_add_eid_al_adha_day(109-120)_add_eid_al_adha_day_two(122-130)_add_eid_al_adha_day_three(132-140)holidays/holiday_base.py (1)
HolidayBase(57-1275)
🔇 Additional comments (8)
holidays/countries/oman.py (8)
1-12: Header and license look good.
No concerns or enhancements noted here.
13-35: Imports are consistent with library structure.
They adhere to the established patterns in the codebase. No issues found.
37-47: Docstring references are helpful.
Everything is well-documented with relevant links to decrees. This should guide future maintainers effectively.
49-55: Class-level settings appear accurate.
The default language, start year, and multiple language support are all aligned with Oman’s holidays.
56-67: Constructor usage is straightforward.
Combining the custom Islamic calendar class with standard inheritance meets the library’s pattern. Nice job.
68-114: Holiday population logic is clear.
The separation of holiday additions (pre-2020 vs post-2020) and Islamic holiday handling is consistent. Appears robust for multi-day events.
116-122: Alias classes are standard practice.
The aliasesOMandOMNare typical. All good.
124-186: Date dictionaries cover key years.
Defining known Islamic date mappings is helpful. Consider extending future years or referencing official sources for ongoing accuracy.
Signed-off-by: Wasif Shahzad <159248937+Wasif-Shahzad@users.noreply.github.com>
|
I've added Oman's Holidays.
Resolves #1237.
Type of change
holidaysfunctionality in general)Checklist
make check, all checks and tests are green