+
Skip to content

Conversation

PPsyrius
Copy link
Collaborator

@PPsyrius PPsyrius commented Jul 24, 2025

Proposed change

  • Add 2025 observances for Ashura and Eid al-Adha dates for Pakistan.
  • Refactor Pakistan Islamic holidays test cases to library-wide standard as of July 2025 i.e. Update Egypt holidays #2742 .

(Issue initially raised by @Wasif-Shahzad on Slack)

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 24, 2025

Summary by CodeRabbit

  • Bug Fixes

    • Updated and finalized 2025 holiday dates for Pakistan, including Ashura, Eid al-Adha, and Eid al-Fitr.
    • Removed "(estimated)" labels from 2025 holiday entries, reflecting confirmed dates.
  • Documentation

    • Added a clarifying comment regarding Pakistan's independence date.
  • Tests

    • Simplified and improved holiday tests for Pakistan, ensuring accurate coverage for 2025 and streamlining test code.

"""

Walkthrough

This update adds 2025 holiday data for Pakistan, updates holiday date sources, and refines test coverage. Holiday JSON snapshots are revised to confirm 2025 dates. Tests are refactored for clarity, using explicit date strings and consolidated year ranges, with some redundant imports and code patterns removed.

Changes

Files/Groups Change Summary
holidays/countries/pakistan.py Added 2025 dates for Islamic holidays, updated archive URLs, clarified comments, no logic change.
snapshots/countries/PK_COMMON.json Updated 2025 Eid al-Adha and Ashura dates from estimated to confirmed, removed "(estimated)".
tests/countries/test_pakistan.py Refactored tests: unified year ranges, replaced loops/timedelta with explicit date strings, removed unused imports.

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~8 minutes

Suggested labels

l10n, test

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 c140f80 and f82760a.

📒 Files selected for processing (1)
  • holidays/countries/pakistan.py (5 hunks)
🧰 Additional context used
🧠 Learnings (2)
📓 Common learnings
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#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: 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#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#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: 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: 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: PPsyrius
PR: vacanza/holidays#2386
File: tests/countries/test_nepal.py:499-536
Timestamp: 2025-04-05T06:49:06.217Z
Learning: In the holidays project, test files follow a dual testing approach: individual methods test specific holidays across multiple years, while comprehensive year-specific tests (e.g., `test_2025`) verify all holidays for a specific year in a single assertion. Both approaches serve different testing purposes and complement each other.
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: 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: 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.
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: PPsyrius
PR: vacanza/holidays#2489
File: holidays/countries/sao_tome_and_principe.py:22-26
Timestamp: 2025-04-23T14:55:35.504Z
Learning: References in holidays classes should only be included if they're used for test case cross-checks or provide historical context about when holidays were added/removed, not just for the sake of having more references.
Learnt from: PPsyrius
PR: vacanza/holidays#2489
File: holidays/countries/sao_tome_and_principe.py:22-26
Timestamp: 2025-04-23T14:55:35.504Z
Learning: References in holidays classes should only be included if they're used for test case cross-checks or provide historical context about when holidays were added/removed, not just for the sake of having more references.
Learnt from: PPsyrius
PR: vacanza/holidays#2642
File: tests/countries/test_france.py:24-27
Timestamp: 2025-06-18T10:10:46.158Z
Learning: The holidays library prioritizes full test coverage over performance in test suites. Modern library-wide standards favor comprehensive testing across all subdivisions and wide year ranges (e.g., 1803-2050 for France) to ensure thorough validation, even if it creates many test objects.
holidays/countries/pakistan.py (22)

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.

Learnt from: PPsyrius
PR: #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: #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: #2537
File: holidays/countries/finland.py:0-0
Timestamp: 2025-05-10T04:32:15.760Z
Learning: In the holidays package, detailed historical context and additional information should be added as comments at the method level or above conditional blocks, while comments directly above tr() function calls should only contain the holiday name itself (e.g., "# Independence Day.").

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: 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: 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: PPsyrius
PR: #2489
File: holidays/countries/sao_tome_and_principe.py:22-26
Timestamp: 2025-04-23T14:55:35.504Z
Learning: References in holidays classes should only be included if they're used for test case cross-checks or provide historical context about when holidays were added/removed, not just for the sake of having more references.

Learnt from: PPsyrius
PR: #2489
File: holidays/countries/sao_tome_and_principe.py:22-26
Timestamp: 2025-04-23T14:55:35.504Z
Learning: References in holidays classes should only be included if they're used for test case cross-checks or provide historical context about when holidays were added/removed, not just for the sake of having more references.

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: #2402
File: holidays/countries/trinidad_and_tobago.py:25-25
Timestamp: 2025-04-02T04:54:07.204Z
Learning: For English-speaking countries in the holidays library (like Trinidad and Tobago), the default_language and supported_languages parameters are optional. If languages are added for such countries, ensure that en_US is included in the list alongside the regional code variant (e.g., en_TT for Trinidad and Tobago).

Learnt from: Wasif-Shahzad
PR: #2593
File: holidays/countries/senegal.py:102-147
Timestamp: 2025-06-04T10:05:58.754Z
Learning: For Senegal holidays: Ashura (10 Muharram) in 2024 falls on July 17th, not July 16th. The user Wasif-Shahzad confirmed this date is correct for Senegal specifically, as Islamic holiday dates can vary by country based on local religious authorities and moon sighting practices.

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: 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: 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: #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: #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: PPsyrius
PR: #2706
File: holidays/countries/cayman_islands.py:80-97
Timestamp: 2025-07-10T03:36:16.461Z
Learning: In the holidays library, date dictionaries that map years to specific dates (like queens_birthday_dates, spring_bank_dates, thanksgiving_day_dates, etc.) are typically defined within the _populate_public_holidays method rather than as module-level constants. This is the established library-wide pattern seen across multiple country implementations including United Kingdom, United States, Sri Lanka, and others.

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: PPsyrius
PR: #2706
File: holidays/countries/cayman_islands.py:80-97
Timestamp: 2025-07-10T03:36:16.461Z
Learning: In the holidays library, date dictionaries that map years to specific dates (like queens_birthday_dates, special holiday dates, etc.) are typically defined within the _populate_public_holidays method rather than as module-level constants. This is the established library-wide pattern and should be maintained for consistency.

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.

⏰ 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 build on ubuntu-latest
  • GitHub Check: Test build on windows-latest
🔇 Additional comments (4)
holidays/countries/pakistan.py (4)

35-35: Independence-day note accepted

The historical clarification is concise and sits in the right spot.


134-157: Eid al-Adha 2025 entry OK, year ordering preserved

Dictionary remains strictly ascending; no structural issues spotted.


159-182: Eid al-Fitr 2025 entry added — verify downstream tests include Mar 31

Everything matches the project’s constant-date pattern.
Just confirm the refactored test suite asserts 2025-03-31.


109-132: 2025 Ashura entry is unique—no duplicates detected
I searched across all Python files and confirmed that holidays/countries/pakistan.py only has one 2025 entry in its ASHURA_DATES block (line 131). The other 2025: occurrences in that file (lines 156 and 181) belong to different holiday mappings.

✨ 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 24, 2025

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 100.00%. Comparing base (305b3d6) to head (00f78af).
Report is 2 commits behind head on dev.

Additional details and impacted files
@@            Coverage Diff            @@
##               dev     #2746   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files          271       271           
  Lines        16199     16199           
  Branches      2190      2190           
=========================================
  Hits         16199     16199           

☔ 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

@Wasif-Shahzad Wasif-Shahzad left a comment

Choose a reason for hiding this comment

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

I'm not supposed to review but this came into my sight right away.

Co-authored-by: Wasif Shahzad <159248937+Wasif-Shahzad@users.noreply.github.com>
Signed-off-by: Panpakorn Siripanich <19505219+PPsyrius@users.noreply.github.com>
Copy link

Copy link
Collaborator

@KJhellico KJhellico 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
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.

LGTM

@arkid15r arkid15r added this pull request to the merge queue Jul 24, 2025
Merged via the queue into vacanza:dev with commit 3ff3c74 Jul 24, 2025
33 checks passed
@PPsyrius PPsyrius deleted the pk_refactor branch July 26, 2025 02:37
@KJhellico KJhellico mentioned this pull request Aug 4, 2025
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.

4 participants

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