Update India holidays: add Parsi New Year holiday (GJ, MH)#3446
Conversation
Implemented a dynamic 365-day drift formula to calculate the Shahenshahi calendar New Year. This ensures accuracy through 2099 by accounting for the 1-day shift every 4 years relative to the Gregorian calendar.
|
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:
WalkthroughAdded a Parsi New Year (Shahenshahi) helper to the Hindu calendar that computes the date from an Aug 27 reference and registered this holiday for Gujarat (GJ) and Maharashtra (MH). Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes 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)
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 |
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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/india.py`:
- Line 14: Remove the leftover development comment on the import line: change
the import statement that currently reads "from datetime import date, timedelta
# Add timedelta here too" to a clean import without the inline comment, keeping
"date" and "timedelta" imported; ensure there are no other stray development
comments in the top of the module.
- Around line 159-170: In _add_parsi_new_year remove the local import of date
and timedelta (they are already imported at module level) and delete the
duplicated call to self._add_holiday(name, dt) so only a single
self._add_holiday(name, dt) remains; keep the leap calculation and dt assignment
as-is in the _add_parsi_new_year method.
🪄 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: 3e708921-4884-4747-bda9-641a444987f6
📒 Files selected for processing (1)
holidays/countries/india.py
|
@pratikpanda2006, can you provide any sources with a more detailed description of this calendar? |
|
Hi @KJhellico, To provide more technical context on the Parsi New Year (Shahenshahi) implementation: the logic is based on the Zoroastrian religious calendar, which operates on a fixed 365-day cycle with no leap year adjustments. This causes the holiday to 'drift' one day forward in the Gregorian calendar every four years. Technical Verification & Logic:
|
https://en.wikipedia.org/wiki/Zoroastrian_calendar#Shahanshahi_calendar - is this it? |
|
@KJhellico, |
|
@KJhellico |
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
holidays/countries/india.py (2)
347-347:⚠️ Potential issue | 🟡 MinorMissing l10n comment above holiday addition.
Per project convention, translatable strings need a comment in the line above for localization purposes. This was previously flagged.
Required fix
self._add_holiday_oct_31(tr("Sardar Vallabhbhai Patel Jayanti")) + + # Parsi New Year (Shahenshahi). self._add_parsi_new_year(tr("Parsi New Year (Shahenshahi)"))Remember to run
make l10nand add translations to all supported language.pofiles.,
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@holidays/countries/india.py` at line 347, Add the missing localization comment immediately above the self._add_parsi_new_year(tr("Parsi New Year (Shahenshahi)")) call: insert the project-standard translator comment (e.g. a brief "Translators:" comment describing the holiday name/context) on the line above the tr(...) usage so the l10n tools pick it up, then run make l10n and update the .po files for all supported languages.
410-410:⚠️ Potential issue | 🟡 MinorMissing l10n comment above holiday addition.
Same issue as in the Gujarat subdivision. Add the comment for localization consistency.
Required fix
self._add_holiday_may_1(tr("Maharashtra Day")) + + # Parsi New Year (Shahenshahi). self._add_parsi_new_year(tr("Parsi New Year (Shahenshahi)"))🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@holidays/countries/india.py` at line 410, Add the missing localization comment above the Parsi New Year addition: insert the same l10n comment used in the Gujarat subdivision immediately above the self._add_parsi_new_year(tr("Parsi New Year (Shahenshahi)")) call so the string has the required localization context for translators and tools.
🤖 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/calendars/hindu.py`:
- Around line 1590-1602: The drift calculation in _add_parsi_new_year is wrong
because leaps = (self._year - 1970) // 4 miscounts actual Gregorian leap years;
change it to compute the number of leap years between 1970 (inclusive) and
self._year (exclusive) using calendar.isleap (e.g., count = sum(1 for y in
range(1970, self._year) if calendar.isleap(y))) and use that count when calling
_timedelta(date(self._year, 8, 28), -count) before calling
self._add_holiday(name, ...); also add the return type annotation to the
function signature (def _add_parsi_new_year(self, name) -> None:).
---
Duplicate comments:
In `@holidays/countries/india.py`:
- Line 347: Add the missing localization comment immediately above the
self._add_parsi_new_year(tr("Parsi New Year (Shahenshahi)")) call: insert the
project-standard translator comment (e.g. a brief "Translators:" comment
describing the holiday name/context) on the line above the tr(...) usage so the
l10n tools pick it up, then run make l10n and update the .po files for all
supported languages.
- Line 410: Add the missing localization comment above the Parsi New Year
addition: insert the same l10n comment used in the Gujarat subdivision
immediately above the self._add_parsi_new_year(tr("Parsi New Year
(Shahenshahi)")) call so the string has the required localization context for
translators and tools.
🪄 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: 285c4482-df70-4bd9-8e6c-725c57b10814
📒 Files selected for processing (2)
holidays/calendars/hindu.pyholidays/countries/india.py
There was a problem hiding this comment.
♻️ Duplicate comments (1)
holidays/countries/india.py (1)
349-349:⚠️ Potential issue | 🟡 MinorAdd missing holiday-name l10n comments above both new
tr()calls.Both new holiday additions should have a direct holiday-name comment line immediately above them. This keeps translator metadata correct and consistent.
Proposed fix
# Sardar Vallabhbhai Patel Jayanti. self._add_holiday_oct_31(tr("Sardar Vallabhbhai Patel Jayanti")) + # Parsi New Year (Shahenshahi). self._add_parsi_new_year(tr("Parsi New Year (Shahenshahi)")) @@ # Maharashtra Day. self._add_holiday_may_1(tr("Maharashtra Day")) + # Parsi New Year (Shahenshahi). self._add_parsi_new_year(tr("Parsi New Year (Shahenshahi)"))#!/bin/bash # Verify translator comments + msgid presence + test coverage mentions. set -euo pipefail echo "== New holiday call sites in India ==" rg -n 'Parsi New Year \(Shahenshahi\)|_add_parsi_new_year\(' holidays/countries/india.py -C2 echo echo "== Existing locale entries for the new msgid ==" rg -n 'msgid "Parsi New Year \(Shahenshahi\)"' holidays/locale -C1 || true echo echo "== Country tests referencing the new holiday ==" rg -n 'Parsi New Year|Shahenshahi|_add_parsi_new_year|GJ|MH' tests/countries --type=py -C2 || trueBased on learnings: “translator comments (#.) in .po files are auto-generated from holiday name comments in the Python source code… run
make l10nto regenerate the .po files.”Also applies to: 412-412
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@holidays/countries/india.py` at line 349, Add the missing holiday-name translator comments immediately above both tr() calls that add the Parsi New Year holiday; specifically, insert a direct comment line above the _add_parsi_new_year(tr("Parsi New Year (Shahenshahi)")) call (and the other occurrence) using the same holiday-name l10n comment format used elsewhere so the automatic .po translator metadata is generated correctly; locate the calls by the _add_parsi_new_year function name and the tr("Parsi New Year (Shahenshahi)") msgid and add the comment line directly above each call.
🤖 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/countries/india.py`:
- Line 349: Add the missing holiday-name translator comments immediately above
both tr() calls that add the Parsi New Year holiday; specifically, insert a
direct comment line above the _add_parsi_new_year(tr("Parsi New Year
(Shahenshahi)")) call (and the other occurrence) using the same holiday-name
l10n comment format used elsewhere so the automatic .po translator metadata is
generated correctly; locate the calls by the _add_parsi_new_year function name
and the tr("Parsi New Year (Shahenshahi)") msgid and add the comment line
directly above each call.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 2f92b29d-1eed-49b5-b1ab-687a5fc3a6a3
📒 Files selected for processing (1)
holidays/countries/india.py
…panda2006/holidays into feat-india-parsi-new-year
There was a problem hiding this comment.
♻️ Duplicate comments (1)
holidays/calendars/hindu.py (1)
1590-1602:⚠️ Potential issue | 🟠 MajorBase date produces dates off by 1 day; also needs return type.
Testing against the PR's verification examples:
Year Current Result Expected 2024 Aug 14 Aug 15 2026 Aug 14 Aug 15 2028 Aug 13 Aug 14 2099 Jul 27 Jul 28 The formula looks correct, but the base date should be August 28, not August 27. Additionally, add the return type annotation per static analysis.
🛠️ Proposed fix
-def _add_parsi_new_year(self, name): +def _add_parsi_new_year(self, name) -> None: # The Parsi New Year (Shahenshahi) follows a 365-day fixed cycle. # It shifts back by 1 day every 4 years relative to the Gregorian calendar. # Base reference: August 15, 2024. - # Using 1970 as a more standard base year: - # In 1970, the holiday fell on August 28. + # Using 1972 as base year with August 28 reference. # Calculate shifts based on leap years since 1970 leaps = (self._year - 1972) // 4 # Use the internal _timedelta helper - dt = _timedelta(date(self._year, 8, 27), -leaps) + dt = _timedelta(date(self._year, 8, 28), -leaps) self._add_holiday(name, dt)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@holidays/calendars/hindu.py` around lines 1590 - 1602, The _add_parsi_new_year function uses the wrong base date and lacks a return type: change the base reference from date(self._year, 8, 27) to date(self._year, 8, 28) so computed results match expected examples, ensure the leap calculation stays (leaps = (self._year - 1972) // 4), and add an explicit return type annotation (e.g., -> None) to the _add_parsi_new_year signature; keep using _timedelta and self._add_holiday as-is.
🤖 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/calendars/hindu.py`:
- Around line 1590-1602: The _add_parsi_new_year function uses the wrong base
date and lacks a return type: change the base reference from date(self._year, 8,
27) to date(self._year, 8, 28) so computed results match expected examples,
ensure the leap calculation stays (leaps = (self._year - 1972) // 4), and add an
explicit return type annotation (e.g., -> None) to the _add_parsi_new_year
signature; keep using _timedelta and self._add_holiday as-is.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 1431d487-5b03-40da-879a-07c87949898e
📒 Files selected for processing (1)
holidays/calendars/hindu.py
|
"Hi @KJhellico sir, I’ve updated the PR with the following: Moved _add_parsi_new_year to HinduCalendarHolidays in hindu.py. Refactored the logic to use the _timedelta helper with 1972 as the base year (verified to align perfectly with the Gregorian calendar and fix the 2026 drift). Added the missing return type annotation (-> None). Added the required l10n comments in india.py for both MH and GJ. Cleaned up unused imports and verified all pre-commit hooks pass locally. Please see to it sir |
|
@KJhellico |
…2018 and local translations
Co-authored-by: ~Jhellico <KJhellico@users.noreply.github.com> Signed-off-by: PRATIK PANDA <pratikpanda2006@gmail.com>
|
@KJhellico |
Co-authored-by: ~Jhellico <KJhellico@users.noreply.github.com> Signed-off-by: PRATIK PANDA <pratikpanda2006@gmail.com>
updaating format of test Co-authored-by: ~Jhellico <KJhellico@users.noreply.github.com> Signed-off-by: PRATIK PANDA <pratikpanda2006@gmail.com>
|
@KJhellico sir have made necessary changes and will use updated format of test and methods henceforth .Thank you. |
👆 |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## dev #3446 +/- ##
=========================================
Coverage 100.00% 100.00%
=========================================
Files 312 312
Lines 18644 18651 +7
Branches 2383 2383
=========================================
+ Hits 18644 18651 +7 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
@KJhellico sir see i have moved the method as instructed by u above test_l10n_default.is it correct now? |
|
Thank you @KJhellico sir and @PPsyrius sir for the detailed feedback and the approvals! I've learned a lot about the library's standards through this process and will try to contribute more.Looking forward to the merge. |
|
arkid15r
left a comment
There was a problem hiding this comment.
Thanks for updating this @pratikpanda2006
|
Thank you @KJhellico, @PPsyrius, and @arkid15r for the thorough reviews and the mentorship throughout this PR. I’ve learned a lot about the library’s standards and the importance of precise edge-case testing. Excited to see this merged and working to get more such prs merged in the org 🙂. |
Proposed change
Added Parsi New Year (Shahenshahi) for India, specifically for the Gujarat (GJ) and Maharashtra (MH) subdivisions.
Logic: The Shahenshahi calendar follows a 365-day year without leap years, resulting in a 1-day drift every 4 years relative to the Gregorian calendar. I implemented this dynamically using a base reference of August 15, 2024, to ensure accuracy through 2099.
Verification:
I verified the drift logic across multiple years:
2024: August 15
2026: August 15
2028: August 14 (Correct 1-day shift)
2099: July 28
Type of change
[x] Supported country/market holidays update (calendar discrepancy fix, localization)
Checklist
[x] I've read and followed the contributing guidelines