Update Pakistan holidays: add OPTIONAL category support#3331
Update Pakistan holidays: add OPTIONAL category support#3331SaadBazaz wants to merge 7 commits intovacanza:devfrom
OPTIONAL category support#3331Conversation
Updated according to Cabinet Division announcement. Signed-off-by: Saad A. Bazaz <bazaz@grayhat.studio>
SaadBazaz
requested review from
KJhellico,
PPsyrius and
arkid15r
as code owners
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughExtends Pakistan holidays with Christian, Hindu, and Buddhist subsystems, enables OPTIONAL category, adds many optional/estimated holidays (with 2024 examples), adds Day after Christmas, updates PakistanIslamicHolidays with confirmed 2026 dates, and updates locales, README, and tests. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
📝 Coding Plan
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment Tip CodeRabbit can use OpenGrep to find security vulnerabilities and bugs across 17+ programming languages.OpenGrep is compatible with Semgrep configurations. Add an |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (4)
holidays/countries/pakistan.py (4)
153-167:⚠️ Potential issue | 🟡 MinorUpdate
EID_AL_FITR_DATES_CONFIRMED_YEARSto include 2026.Update the confirmed years range to
(2005, 2026).Proposed fix
- EID_AL_FITR_DATES_CONFIRMED_YEARS = (2005, 2025) + EID_AL_FITR_DATES_CONFIRMED_YEARS = (2005, 2026)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@holidays/countries/pakistan.py` around lines 153 - 167, The tuple constant EID_AL_FITR_DATES_CONFIRMED_YEARS currently ends at 2025 but 2026 appears in EID_AL_FITR_DATES; update EID_AL_FITR_DATES_CONFIRMED_YEARS to include 2026 (change from (2005, 2025) to (2005, 2026)) so the confirmed-years range matches the entries in EID_AL_FITR_DATES.
132-149:⚠️ Potential issue | 🟡 MinorUpdate
EID_AL_ADHA_DATES_CONFIRMED_YEARSto include 2026.Same as above - update the confirmed years range to
(2005, 2026).Proposed fix
- EID_AL_ADHA_DATES_CONFIRMED_YEARS = (2005, 2025) + EID_AL_ADHA_DATES_CONFIRMED_YEARS = (2005, 2026)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@holidays/countries/pakistan.py` around lines 132 - 149, Update the confirmed-year range for Eid al-Adha by changing the EID_AL_ADHA_DATES_CONFIRMED_YEARS tuple from (2005, 2025) to (2005, 2026) so it includes the newly listed 2026 entry in EID_AL_ADHA_DATES; modify the constant EID_AL_ADHA_DATES_CONFIRMED_YEARS accordingly in the file where it's defined.
117-128:⚠️ Potential issue | 🟡 MinorUpdate
ASHURA_DATES_CONFIRMED_YEARSto include 2026.You've added 2026 entries to
ASHURA_DATES, butASHURA_DATES_CONFIRMED_YEARSstill shows(2005, 2025). Update the range to reflect the new confirmed year.Proposed fix
- ASHURA_DATES_CONFIRMED_YEARS = (2005, 2025) + ASHURA_DATES_CONFIRMED_YEARS = (2005, 2026)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@holidays/countries/pakistan.py` around lines 117 - 128, The constant ASHURA_DATES_CONFIRMED_YEARS is out of sync with ASHURA_DATES—add 2026 to the confirmed range by updating the tuple ASHURA_DATES_CONFIRMED_YEARS (currently (2005, 2025)) so it includes 2026 to match the ASHURA_DATES entries in pakistan.py.
171-191:⚠️ Potential issue | 🟠 MajorUpdate
MAWLID_DATES_CONFIRMED_YEARSto reflect dictionary entries and clarify 2025.
MAWLID_DATESincludes 2026 butMAWLID_DATES_CONFIRMED_YEARSonly extends to 2024. Either add 2026 to the confirmed years range, or use explicit confirmed dates likeEID_AL_FITR_DATES_CONFIRMEDdoes. Also, the 2025 entry is missing from the dictionary—verify whether this is intentional (pending official Pakistan reference) or an oversight; if intentional, consider documenting why.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@holidays/countries/pakistan.py` around lines 171 - 191, MAWLID_DATES_CONFIRMED_YEARS currently stops at 2024 while MAWLID_DATES contains a 2026 entry and omits 2025; update the confirmed-years representation to match the data by either adding 2026 to MAWLID_DATES_CONFIRMED_YEARS or replace the range with an explicit set (like EID_AL_FITR_DATES_CONFIRMED) listing confirmed years, and if 2025 is intentionally absent add a clear inline comment/documentation explaining that 2025 is pending official confirmation so future maintainers know it was deliberate.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@holidays/countries/pakistan.py`:
- Around line 101-104: The call in _populate_optional_holidays passes args to
_add_holiday in the wrong order: it currently calls self._add_holiday(month,
day, name) but _add_holiday expects the holiday name first and date components
after; change the call to pass the name first (e.g., self._add_holiday(name,
month, day)) when iterating over PakistanOptionalHolidays.generate(self._year)
so the holiday is stored with the correct name.
- Line 52: The __init__ currently calls self._populate_optional_holidays(),
which breaks the HolidayBase population flow because self._year isn't set and
PakistanOptionalHolidays.generate(self._year) fails; remove the call to
self._populate_optional_holidays() from the __init__ method and rely on the
HolidayBase class to invoke _populate_optional_holidays() during year population
so PakistanOptionalHolidays.generate can receive a valid self._year.
- Around line 197-198: Add an explicit return type annotation to the static
generator method generate to improve readability and type checking: change the
signature of generate(year) to include a generator/iterator type (for example
def generate(year) -> Iterator[date]: or -> Generator[date, None, None]:), and
ensure you import the required types (from typing import Iterator or Generator
and from datetime import date) at the top of the module; reference the
staticmethod generate to make this change.
- Around line 194-225: The generator PakistanOptionalHolidays.generate currently
yields many lunar-based holiday dates (e.g., "Holi", "Diwali", "Baisakhi",
"Birthday of Guru Nanak Dev Ji") as fixed dates for every year; make these
2026-specific by wrapping those lunar/variable-date yield statements in a year
check (e.g., if year == 2026:) or alternatively replace them with year-indexed
date maps similar to CustomIslamicHolidays — locate
PakistanOptionalHolidays.generate and either add per-year dictionaries for these
Hindu/Sikh lunar holidays or guard the existing yields so they only execute for
year == 2026.
---
Outside diff comments:
In `@holidays/countries/pakistan.py`:
- Around line 153-167: The tuple constant EID_AL_FITR_DATES_CONFIRMED_YEARS
currently ends at 2025 but 2026 appears in EID_AL_FITR_DATES; update
EID_AL_FITR_DATES_CONFIRMED_YEARS to include 2026 (change from (2005, 2025) to
(2005, 2026)) so the confirmed-years range matches the entries in
EID_AL_FITR_DATES.
- Around line 132-149: Update the confirmed-year range for Eid al-Adha by
changing the EID_AL_ADHA_DATES_CONFIRMED_YEARS tuple from (2005, 2025) to (2005,
2026) so it includes the newly listed 2026 entry in EID_AL_ADHA_DATES; modify
the constant EID_AL_ADHA_DATES_CONFIRMED_YEARS accordingly in the file where
it's defined.
- Around line 117-128: The constant ASHURA_DATES_CONFIRMED_YEARS is out of sync
with ASHURA_DATES—add 2026 to the confirmed range by updating the tuple
ASHURA_DATES_CONFIRMED_YEARS (currently (2005, 2025)) so it includes 2026 to
match the ASHURA_DATES entries in pakistan.py.
- Around line 171-191: MAWLID_DATES_CONFIRMED_YEARS currently stops at 2024
while MAWLID_DATES contains a 2026 entry and omits 2025; update the
confirmed-years representation to match the data by either adding 2026 to
MAWLID_DATES_CONFIRMED_YEARS or replace the range with an explicit set (like
EID_AL_FITR_DATES_CONFIRMED) listing confirmed years, and if 2025 is
intentionally absent add a clear inline comment/documentation explaining that
2025 is pending official confirmation so future maintainers know it was
deliberate.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: fcaccf5f-5989-44ee-9f5a-740a1d580c9c
📒 Files selected for processing (1)
holidays/countries/pakistan.py
holidays/countries/pakistan.py
Outdated
| @staticmethod | ||
| def generate(year): |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Consider adding return type annotation.
The generator could benefit from a type hint for clarity.
Suggested improvement
+from typing import Generator
+
`@staticmethod`
- def generate(year):
+ def generate(year: int) -> Generator[tuple[int, int, str], None, None]:🧰 Tools
🪛 Ruff (0.15.5)
[warning] 198-198: Missing return type annotation for staticmethod generate
(ANN205)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@holidays/countries/pakistan.py` around lines 197 - 198, Add an explicit
return type annotation to the static generator method generate to improve
readability and type checking: change the signature of generate(year) to include
a generator/iterator type (for example def generate(year) -> Iterator[date]: or
-> Generator[date, None, None]:), and ensure you import the required types (from
typing import Iterator or Generator and from datetime import date) at the top of
the module; reference the staticmethod generate to make this change.
|
@SaadBazaz, I would advise you to look at examples from other countries to see how OPTIONAL category should be properly implemented. |
…uite, update README
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (1)
holidays/countries/pakistan.py (1)
133-166:⚠️ Potential issue | 🟠 MajorDon't reuse one year's movable optional dates across the full supported range.
This block bakes year-specific dates into every year from 1948 onward. Basant Panchami, Shiv Ratri, Holi, Buddha Purnima, Krishna Janmashtami, Dussehra, and the Guru birthdays move year to year, so these literals will be wrong outside the source year. At minimum, gate the fixed-date entries to the officially announced year until there is per-year/custom-calendar data for them.
🛠️ Minimal containment fix
- # Hindu & Sikh holidays (Optional) - self._add_holiday(tr("Basant Panchami"), JAN, 23) - self._add_holiday(tr("Shiv Ratri"), FEB, 16) - self._add_holiday(tr("Dulhandi"), MAR, 3) - self._add_holiday(tr("Holi"), MAR, 4) + if self._year == 2026: + self._add_holiday(tr("Basant Panchami"), JAN, 23) + self._add_holiday(tr("Shiv Ratri"), FEB, 16) + self._add_holiday(tr("Dulhandi"), MAR, 3) + self._add_holiday(tr("Holi"), MAR, 4)Apply the same treatment to the remaining fixed-date optional entries in this block.
Based on learnings: exact holiday dates in this codebase are only kept for years with official sources, and
_CustomIslamicHolidaysstores only verified dates rather than reusing year-specific values across years.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@holidays/countries/pakistan.py` around lines 133 - 166, The block currently adds movable-religious holidays as fixed dates for every year via calls like _add_holiday(...), _add_diwali(...) and specific entries (e.g., "Basant Panchami", "Holi", "Buddha Purnima", "Krishna Janmashtami", "Dussehra", "Birthday of Guru Nanak Dev Ji"); change these so year-specific dates are not applied across the full supported range—either restrict each fixed-date call to the officially announced year(s) (e.g., guard with if self._year == <official_year> or if self._year in {...}) or remove them and register those dates only from the per-year/custom-calendar source (e.g., move into the verified-dates structure similar to _CustomIslamicHolidays); apply the same gating to all optional fixed entries in this block so only verified years get hardcoded dates.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@holidays/countries/pakistan.py`:
- Around line 118-131: The comments in _populate_optional_holidays are leaking
into translation files; move the explanatory/category comments (e.g., "These are
optional holidays..." and the note about Islamic optional holidays being
estimated and using helpers) to a single comment block above the whole
optional-holidays section, then ensure each line immediately above a tr(...)
call contains only the exact holiday name (no "(Optional)" labels or
implementation notes) so that comments before _add_new_years_day,
_add_holiday(tr(...)) and similar calls match the holiday text exactly; keep
references to functions _populate_optional_holidays, _add_new_years_day,
_add_holiday and tr so you can locate the places to change.
In `@holidays/locale/en_PK/LC_MESSAGES/PK.po`:
- Around line 31-160: This file uses the project's default-locale convention
incorrectly by duplicating source text into msgstr; for en_PK you must clear all
msgstr values so the source strings remain only in msgid (e.g., entries like
"Pakistan Day", "Labour Day", "Eid-ul-Fitr", "Shab-e-Meraj (estimated)",
"Diwali", etc. should keep their msgid text and have empty msgstr fields);
update every msgstr in this PO file to an empty string while leaving the msgid
lines intact so en_PK remains the source catalog.
In `@tests/countries/test_pakistan.py`:
- Around line 64-184: Tests only cover 2024 but PakistanIslamicHolidays was
updated for 2026; add a small year-specific assertion to prevent regressions by
creating a new test (e.g., test_l10n_2026) or extend an existing test
(test_l10n_default or test_l10n_en_us) to assert the 2026 dates for the new
Cabinet Division entries: Eid al-Fitr (2026 dates), Eid al-Adha (2026 dates),
Ashura (2026 dates) and Mawlid/Prophet's Birthday (2026 date) using the same
assertLocalizedHolidays helper so the PakistanIslamicHolidays logic is exercised
for 2026. Ensure you reference the PakistanIslamicHolidays change by adding the
four explicit ("YYYY-MM-DD", "Holiday Name") tuples for 2026 in the same
localized variants (default/en_US/ur_PK) you already test.
---
Duplicate comments:
In `@holidays/countries/pakistan.py`:
- Around line 133-166: The block currently adds movable-religious holidays as
fixed dates for every year via calls like _add_holiday(...), _add_diwali(...)
and specific entries (e.g., "Basant Panchami", "Holi", "Buddha Purnima",
"Krishna Janmashtami", "Dussehra", "Birthday of Guru Nanak Dev Ji"); change
these so year-specific dates are not applied across the full supported
range—either restrict each fixed-date call to the officially announced year(s)
(e.g., guard with if self._year == <official_year> or if self._year in {...}) or
remove them and register those dates only from the per-year/custom-calendar
source (e.g., move into the verified-dates structure similar to
_CustomIslamicHolidays); apply the same gating to all optional fixed entries in
this block so only verified years get hardcoded dates.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 1f1ae6aa-ccf3-4d03-b9d2-16aff1fd77d1
📒 Files selected for processing (6)
README.mdholidays/countries/pakistan.pyholidays/locale/en_PK/LC_MESSAGES/PK.poholidays/locale/en_US/LC_MESSAGES/PK.poholidays/locale/ur_PK/LC_MESSAGES/PK.potests/countries/test_pakistan.py
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
tests/countries/test_pakistan.py (1)
24-30:⚠️ Potential issue | 🟡 MinorAdd
test_no_holidaysfor all supported categories.With OPTIONAL now supported, pre-start validation should explicitly cover both categories, not just default PUBLIC behavior.
✅ Suggested test addition
class TestPakistan(CommonCountryTests, TestCase): @@ def setUpClass(cls): years = range(1948, 2050) super().setUpClass(Pakistan, years=years) cls.no_estimated_holidays = Pakistan(years=years, islamic_show_estimated=False) + + def test_no_holidays(self): + self.assertNoHolidays( + Pakistan, + years=(self.start_year - 1,), + categories=Pakistan.supported_categories, + )Based on learnings:
test_no_holidaysshould validate allsupported_categorieswhen non-PUBLIC categories (like OPTIONAL) are supported.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/countries/test_pakistan.py` around lines 24 - 30, The current tests only validate the default PUBLIC category for the "no holidays" case; add a new test method (e.g., test_no_holidays) on the TestPakistan class that iterates over Pakistan.supported_categories and for each category constructs Pakistan(years=years, islamic_show_estimated=False, categories=[category]) (or uses cls.no_estimated_holidays with category override) and asserts that pre-start validation yields no holidays for that category; reference TestPakistan, setUpClass, cls.no_estimated_holidays, Pakistan, supported_categories and ensure the test covers all supported categories including OPTIONAL rather than only PUBLIC.
♻️ Duplicate comments (4)
tests/countries/test_pakistan.py (1)
69-150:⚠️ Potential issue | 🟡 MinorPlease add targeted 2026 regression assertions for updated Islamic dates.
The PR adds 2026 confirmed Islamic dates, but current localized snapshots only validate 2024. Add a small 2026 block for Eid al-Fitr, Eid al-Adha, Ashura, and Mawlid/Prophet’s Birthday.
Based on learnings: this project intentionally combines broad snapshots with targeted year-specific assertions for newly introduced holiday-date changes.
Also applies to: 152-192
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/countries/test_pakistan.py` around lines 69 - 150, Add targeted 2026 assertions to the localized holiday tests by extending test_l10n_default and test_l10n_en_us to include a small block asserting the confirmed 2026 Islamic dates: Eid al-Fitr (all observed days in 2026), Eid al-Adha (all observed days in 2026), Ashura (both observed dates in 2026), and Mawlid/Prophet's Birthday (Prophet's Birthday) using the existing assertLocalizedHolidays helper so the snapshots still cover broad 2024 checks while explicitly validating the new 2026 regressions.holidays/countries/pakistan.py (2)
98-99:⚠️ Potential issue | 🟡 MinorTranslator comments should be holiday-name-only and punctuation-consistent.
Category headers like “Hindu & Sikh holidays” and comments without trailing period (e.g., Line 98, Line 123) are extracted into
.pofiles and pollute translator hints. Keep only exact holiday names directly above holiday additions; move grouping/context comments above the block.🧹 Minimal cleanup pattern
- # Day after Christmas + # Day after Christmas. self._add_holiday_dec_26(tr("Day after Christmas")) - # New Year's Day + # Optional holidays. + # New Year's Day. self._add_new_years_day(tr("New Year's Day")) - # Hindu & Sikh holidays + # Basant Panchami. self._add_holiday(tr("Basant Panchami"), JAN, 23)Based on learnings: comments immediately above
tr()holiday calls should match holiday names only, with explanatory grouping comments moved outside those lines.Also applies to: 123-123, 126-126, 133-133, 139-139, 144-144, 147-147, 150-150, 153-153, 157-157, 164-164
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@holidays/countries/pakistan.py` around lines 98 - 99, The translator comment above the holiday additions is too verbose and is being extracted into .po files; replace the current comment line immediately above each tr() holiday call (e.g., the comment above self._add_holiday_dec_26(tr("Day after Christmas"))) so it contains only the exact holiday name (no punctuation or extra context), and move any grouping/context comments (like "Hindu & Sikh holidays") to a separate comment placed above the whole block; ensure this change is applied for all similar calls (e.g., occurrences around self._add_holiday_dec_26, other self._add_holiday_* calls that pass tr(...)) so translator hints contain only the holiday names.
133-138:⚠️ Potential issue | 🟠 MajorLunisolar optional holidays are hardcoded as fixed dates across all years.
Line 134 onward adds variable-date holidays (e.g., Holi, Janmashtami, Dussehra, Guru Nanak birthday) with one fixed date, so years outside the source year will be wrong.
🐛 Suggested fix (year-scoped data for verified dates)
- # Hindu & Sikh holidays - self._add_holiday(tr("Basant Panchami"), JAN, 23) - self._add_holiday(tr("Shiv Ratri"), FEB, 16) - self._add_holiday(tr("Dulhandi"), MAR, 3) - self._add_holiday(tr("Holi"), MAR, 4) + # Year-specific optional lunisolar holidays (verified dates only). + lunisolar_optional_dates = { + 2024: ( + (JAN, 23, tr("Basant Panchami")), + (FEB, 16, tr("Shiv Ratri")), + (MAR, 3, tr("Dulhandi")), + (MAR, 4, tr("Holi")), + (SEP, 4, tr("Krishna Janmashtami")), + (OCT, 19, tr("Durga Puja")), + (OCT, 20, tr("Dussehra")), + (OCT, 26, tr("Dussehra Holiday")), + (NOV, 24, tr("Birthday of Guru Nanak Dev Ji")), + (NOV, 30, tr("Birthday of Guru Valmiki Ji")), + ), + } + for month, day, name in lunisolar_optional_dates.get(self._year, ()): + self._add_holiday(name, month, day)Based on learnings: explicit lunar-calendar dates in this codebase are intended to be year-specific verified data, not reused as fixed dates for all years.
Also applies to: 145-161, 165-166
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@holidays/countries/pakistan.py` around lines 133 - 138, The Hindu/Sikh lunisolar holidays in pakistan.py (added via self._add_holiday calls such as tr("Basant Panchami"), tr("Holi"), tr("Dulhandi"), tr("Shiv Ratri"), tr("Janmashtami"), tr("Dussehra"), tr("Guru Nanak birthday") etc.) are hardcoded to fixed month/day values across all years; replace these with year-scoped verified dates by: remove the constant-date self._add_holiday calls for lunisolar holidays and instead load a year-indexed lookup (or conditional blocks) that supplies exact Gregorian dates for each supported year before calling self._add_holiday, ensure the lookup keys include the year and each holiday name used (match tr("...") strings), and fall back gracefully if a year is unsupported; apply the same change pattern for the other blocks noted (the later lunisolar entries) so all lunar-based holidays use year-specific validated data rather than fixed dates.holidays/locale/en_PK/LC_MESSAGES/PK.po (1)
31-163:⚠️ Potential issue | 🟡 Minor
en_PKsource catalog should keepmsgstrempty.Since
en_PKis the default language, duplicating text intomsgstris unnecessary and deviates from project localization convention.🧩 Patch pattern
msgid "Pakistan Day" -msgstr "Pakistan Day" +msgstr ""Based on learnings: for a country’s default locale file,
msgidis the source text andmsgstrentries are intentionally left empty.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@holidays/locale/en_PK/LC_MESSAGES/PK.po` around lines 31 - 163, The en_PK PO entries currently duplicate source text into msgstr (e.g., msgid "Pakistan Day", "Labour Day", "Eid-ul-Fitr", "New Year's Day", "Shab-e-Meraj (estimated)", etc.); for the default locale remove those duplicates by clearing every msgstr to an empty string so msgid remains the source of truth (ensure all occurrences shown in the diff have msgstr "" instead of the copied text).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@tests/countries/test_pakistan.py`:
- Around line 24-30: The current tests only validate the default PUBLIC category
for the "no holidays" case; add a new test method (e.g., test_no_holidays) on
the TestPakistan class that iterates over Pakistan.supported_categories and for
each category constructs Pakistan(years=years, islamic_show_estimated=False,
categories=[category]) (or uses cls.no_estimated_holidays with category
override) and asserts that pre-start validation yields no holidays for that
category; reference TestPakistan, setUpClass, cls.no_estimated_holidays,
Pakistan, supported_categories and ensure the test covers all supported
categories including OPTIONAL rather than only PUBLIC.
---
Duplicate comments:
In `@holidays/countries/pakistan.py`:
- Around line 98-99: The translator comment above the holiday additions is too
verbose and is being extracted into .po files; replace the current comment line
immediately above each tr() holiday call (e.g., the comment above
self._add_holiday_dec_26(tr("Day after Christmas"))) so it contains only the
exact holiday name (no punctuation or extra context), and move any
grouping/context comments (like "Hindu & Sikh holidays") to a separate comment
placed above the whole block; ensure this change is applied for all similar
calls (e.g., occurrences around self._add_holiday_dec_26, other
self._add_holiday_* calls that pass tr(...)) so translator hints contain only
the holiday names.
- Around line 133-138: The Hindu/Sikh lunisolar holidays in pakistan.py (added
via self._add_holiday calls such as tr("Basant Panchami"), tr("Holi"),
tr("Dulhandi"), tr("Shiv Ratri"), tr("Janmashtami"), tr("Dussehra"), tr("Guru
Nanak birthday") etc.) are hardcoded to fixed month/day values across all years;
replace these with year-scoped verified dates by: remove the constant-date
self._add_holiday calls for lunisolar holidays and instead load a year-indexed
lookup (or conditional blocks) that supplies exact Gregorian dates for each
supported year before calling self._add_holiday, ensure the lookup keys include
the year and each holiday name used (match tr("...") strings), and fall back
gracefully if a year is unsupported; apply the same change pattern for the other
blocks noted (the later lunisolar entries) so all lunar-based holidays use
year-specific validated data rather than fixed dates.
In `@holidays/locale/en_PK/LC_MESSAGES/PK.po`:
- Around line 31-163: The en_PK PO entries currently duplicate source text into
msgstr (e.g., msgid "Pakistan Day", "Labour Day", "Eid-ul-Fitr", "New Year's
Day", "Shab-e-Meraj (estimated)", etc.); for the default locale remove those
duplicates by clearing every msgstr to an empty string so msgid remains the
source of truth (ensure all occurrences shown in the diff have msgstr "" instead
of the copied text).
In `@tests/countries/test_pakistan.py`:
- Around line 69-150: Add targeted 2026 assertions to the localized holiday
tests by extending test_l10n_default and test_l10n_en_us to include a small
block asserting the confirmed 2026 Islamic dates: Eid al-Fitr (all observed days
in 2026), Eid al-Adha (all observed days in 2026), Ashura (both observed dates
in 2026), and Mawlid/Prophet's Birthday (Prophet's Birthday) using the existing
assertLocalizedHolidays helper so the snapshots still cover broad 2024 checks
while explicitly validating the new 2026 regressions.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 5edc267d-2cec-4f57-ba27-01c2a15fc3e8
📒 Files selected for processing (5)
holidays/countries/pakistan.pyholidays/locale/en_PK/LC_MESSAGES/PK.poholidays/locale/en_US/LC_MESSAGES/PK.poholidays/locale/ur_PK/LC_MESSAGES/PK.potests/countries/test_pakistan.py
@KJhellico - Checked out some other files and implemented accordingly, to the best of my ability. Would love your review now. |
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
holidays/locale/en_PK/LC_MESSAGES/PK.po (1)
31-163:⚠️ Potential issue | 🟡 MinorKeep
en_PKas source catalog: clear allmsgstrvalues.
holidays/locale/en_PK/LC_MESSAGES/PK.pois a default-locale file, so populatedmsgstrvalues here should be empty; otherwise this drifts from the project’s source-locale pattern.Suggested fix pattern
msgid "Pakistan Day" -msgstr "Pakistan Day" +msgstr "" msgid "Eid-ul-Fitr" -msgstr "Eid-ul-Fitr" +msgstr ""Apply this to all entries in this file (including
"%s (estimated)").Based on learnings: In the Holidays project, .po files for a country's default locale use empty
msgstrfields as a standard convention.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@holidays/locale/en_PK/LC_MESSAGES/PK.po` around lines 31 - 163, This PK.po file is the project's source catalog so all translated strings should be empty; for each msgid (e.g. "%s (estimated)", "Kashmir Solidarity Day", "Labour Day", "Eid-ul-Fitr", "Diwali", etc.) clear the corresponding msgstr to be empty (replace current msgstr values with an empty string for every entry in the file, including the estimated variants) so the file follows the project's default-locale convention.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@holidays/locale/en_US/LC_MESSAGES/PK.po`:
- Around line 64-85: The translator comment lines in the en_US PO file (e.g.,
the comment strings above msgid entries such as "Day after Christmas",
"Eid-ul-Fitr", "Eid-ul-Adha", "Eid Milad-un-Nabi", "Ashura", "New Year's Day")
must end with a period; update each translator comment starting with "#." to
append a trailing period (e.g., change "#. Day after Christmas" to "#. Day after
Christmas.") and apply the same fix across all files under
holidays/locale/en_US/LC_MESSAGES/*.po to enforce consistent punctuation.
- Around line 44-78: The en_US translations are blank causing gettext to fall
back to en_PK msgids; populate the msgstr values with standard US English names
for the non-identical holidays: set msgstr for "Labour Day" to "Labor Day", for
"Eid-ul-Fitr" to "Eid al-Fitr", for "Eid-ul-Adha" to "Eid al-Adha", and for "Eid
Milad-un-Nabi" to a common English form such as "Prophet's Birthday" (leave
other msgids like "Independence Day", "Iqbal Day", "Quaid-e-Azam Day",
"Youm-e-Takbeer", and "Day after Christmas" unchanged unless a specific en_US
variant is required by tests).
---
Duplicate comments:
In `@holidays/locale/en_PK/LC_MESSAGES/PK.po`:
- Around line 31-163: This PK.po file is the project's source catalog so all
translated strings should be empty; for each msgid (e.g. "%s (estimated)",
"Kashmir Solidarity Day", "Labour Day", "Eid-ul-Fitr", "Diwali", etc.) clear the
corresponding msgstr to be empty (replace current msgstr values with an empty
string for every entry in the file, including the estimated variants) so the
file follows the project's default-locale convention.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 5bc51a02-a759-470c-8dcf-c1c8b2d96303
📒 Files selected for processing (3)
holidays/locale/en_PK/LC_MESSAGES/PK.poholidays/locale/en_US/LC_MESSAGES/PK.poholidays/locale/ur_PK/LC_MESSAGES/PK.po
There was a problem hiding this comment.
♻️ Duplicate comments (1)
holidays/locale/en_US/LC_MESSAGES/PK.po (1)
64-64:⚠️ Potential issue | 🟡 MinorAdd trailing periods to the new en_US translator comments.
Several added
#.comment lines miss the final period. Please align with repo punctuation style.Suggested patch
-#. Day after Christmas +#. Day after Christmas. -#. New Year's Day +#. New Year's Day. -#. Hindu & Sikh holidays +#. Hindu & Sikh holidays. -#. Christian holidays +#. Christian holidays. -#. Sikh holidays +#. Sikh holidays. -#. Bahai holidays +#. Bahai holidays. -#. Buddhist holidays +#. Buddhist holidays. -#. Parsi holidays +#. Parsi holidays. -#. Hindu holidays +#. Hindu holidays. -#. Sikh holidays +#. Sikh holidays.Based on learnings: Enforce that message comments in PO files under holidays/locale/en_US/LC_MESSAGES/*.po end with a period.
Also applies to: 84-84, 100-100, 113-113, 123-123, 127-127, 131-131, 135-135, 142-142, 158-158
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@holidays/locale/en_US/LC_MESSAGES/PK.po` at line 64, Update the translator comment lines that start with "#." in the PO file so each ends with a trailing period; e.g., change the comment "Day after Christmas" to "Day after Christmas." and do the same for the other listed comment texts (the ones flagged at the locations corresponding to lines 84, 100, 113, 123, 127, 131, 135, 142, 158) to match the repo punctuation style.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@holidays/locale/en_US/LC_MESSAGES/PK.po`:
- Line 64: Update the translator comment lines that start with "#." in the PO
file so each ends with a trailing period; e.g., change the comment "Day after
Christmas" to "Day after Christmas." and do the same for the other listed
comment texts (the ones flagged at the locations corresponding to lines 84, 100,
113, 123, 127, 131, 135, 142, 158) to match the repo punctuation style.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 75a8f080-455a-4903-a542-b8356f6a39a9
📒 Files selected for processing (1)
holidays/locale/en_US/LC_MESSAGES/PK.po
| self._add_new_years_day(tr("New Year's Day")) | ||
|
|
||
| # Islamic optional holidays (estimated) | ||
| if self._year == 2026: |
There was a problem hiding this comment.
It doesn't make sense to add holidays for a single year only in this way.
There was a problem hiding this comment.
It doesn't make sense to add holidays for a single year only in this way.
@KJhellico - do you recommend I follow the format here:
https://github.com/SaadBazaz/holidays/blob/2300b6e9b5f7f954b2ed35b9376c3595534ebd1f/holidays/countries/pakistan.py#L177-L186
There was a problem hiding this comment.
@SaadBazaz See StaticHolidays example i.e. Singapore's
https://github.com/vacanza/holidays/blob/661581b6eabd4cfa812859a5765b037ef17e8e15/holidays/countries/singapore.py#L257C1-L287C6
| 2018: (SEP, 21), | ||
| 2022: (AUG, 9), | ||
| 2025: (JUL, 6), | ||
| 2026: ((JUN, 24), (JUN, 25)), |
There was a problem hiding this comment.
| 2026: ((JUN, 24), (JUN, 25)), | |
| 2026: (JUN, 25), |
There should be only one day here, 10th Moharram.
There was a problem hiding this comment.
9 and 10 Moharram both are usually off here. (they are combined to be called Ashura)
As per the government notice, at least
There was a problem hiding this comment.
Yes, and this has already been implemented using _add_ashura_eve.
There was a problem hiding this comment.
Do you recommend I also update it for the previous entry?
https://github.com/SaadBazaz/holidays/blob/2300b6e9b5f7f954b2ed35b9376c3595534ebd1f/holidays/countries/pakistan.py#L185C9-L185C13
| #, c-format | ||
| msgid "%s (estimated)" | ||
| msgstr "" | ||
| msgstr "%s (estimated)" |
There was a problem hiding this comment.
In default language .po file msgstrs should be empty.
There was a problem hiding this comment.
Yes. Look at other countries default language's .po files.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## dev #3331 +/- ##
=========================================
Coverage 100.00% 100.00%
=========================================
Files 309 309
Lines 18543 18575 +32
Branches 2377 2378 +1
=========================================
+ Hits 18543 18575 +32 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
Tests need fixing. will do next week. |
| self._add_new_years_day(tr("New Year's Day")) | ||
|
|
||
| # Islamic optional holidays (estimated) | ||
| if self._year == 2026: |
There was a problem hiding this comment.
@SaadBazaz See StaticHolidays example i.e. Singapore's
https://github.com/vacanza/holidays/blob/661581b6eabd4cfa812859a5765b037ef17e8e15/holidays/countries/singapore.py#L257C1-L287C6
| # Buddhist holidays | ||
| self._add_holiday(tr("Buddha Purnima"), MAY, 24) |
There was a problem hiding this comment.
Pretty sure this one is a movable holiday, the same goes for Hindu ones
PPsyrius
changed the title
OPTIONAL category support
Co-authored-by: Panpakorn Siripanich <19505219+PPsyrius@users.noreply.github.com> Signed-off-by: Saad A. Bazaz <bazaz@grayhat.studio>
|
3 participants
Proposed change
Updated the list of Pakistani holidays according to Cabinet division announcement.
Also added optional holidays in there.
Type of change
holidaysfunctionality in general)Checklist
make checklocally; all checks and tests passed.