Update Bangladesh holidays: add Islamic holidays#3531
Conversation
|
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:
WalkthroughBangladesh holiday generator mixes in Christian and Islamic holiday groups, adds islamic_show_estimated/estimated_label, provides Bangladesh-specific confirmed Islamic dates, updates population logic with year gates and new observances, adds en_BD locale and PO updates, and expands tests. ChangesBangladesh holiday logic
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested labels
Suggested reviewers
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Tip 💬 Introducing Slack Agent: The best way for teams to turn conversations into code.Slack Agent is built on CodeRabbit's deep understanding of your code, so your team can collaborate across the entire SDLC without losing context.
Built for teams:
One agent for your entire SDLC. Right inside Slack. 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: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@tests/countries/test_bangladesh.py`:
- Around line 44-46: The test_july_uprising_day currently asserts the holiday
exists from 2024 onward using assertHolidayName but lacks explicit negative
coverage before 2024; add an assertNoHolidayName call for the pre-2024 years
(e.g., using a generator like (f"{year}-08-05" for year in
range(self.full_range.start, 2024) or range(self.min_year, 2024))) to ensure the
holiday is absent prior to its start year and keep the existing
assertHolidayName for the positive 2024+ range.
🪄 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: d8353f34-4746-4e6e-8169-abd4fbb91516
📒 Files selected for processing (5)
holidays/countries/bangladesh.pyholidays/locale/ar/LC_MESSAGES/BD.poholidays/locale/bn/LC_MESSAGES/BD.poholidays/locale/en_US/LC_MESSAGES/BD.potests/countries/test_bangladesh.py
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## dev #3531 +/- ##
=========================================
Coverage 100.00% 100.00%
=========================================
Files 314 314
Lines 18775 18820 +45
Branches 2405 2408 +3
=========================================
+ Hits 18775 18820 +45 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Co-authored-by: Panpakorn Siripanich <19505219+PPsyrius@users.noreply.github.com> Signed-off-by: Shaon Ahamed <shaonahmed8788@gmail.com>
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 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/bangladesh.py`:
- Around line 59-69: Extract the Bangladesh-specific helper logic in
_add_mid_shaban_day and _add_jumuatul_wida and implement shared helpers in
holidays/groups/islamic.py (e.g., functions like add_mid_shaban_day(group,
calendar, year, name) and add_jumatul_wida_day(...)) that encapsulate the call
to the Islamic calendar _get_holiday logic (move the call to
_islamic_calendar._get_holiday into those shared helpers so country files no
longer call the calendar's private API directly). Then update Bangladesh’s
methods to simply call the new shared helpers with the translated names (keeping
_add_islamic_calendar_holiday_set usages in the shared helpers or adjusting to
call the existing helper _add_islamic_calendar_holiday_set as needed), and
remove the duplicated helper implementations from bangladesh.py.
- Around line 117-127: The code currently always adds three consecutive Eid days
by calling _add_eid_al_fitr_day, _add_eid_al_fitr_day_two,
_add_eid_al_fitr_day_three and the analogous _add_eid_al_adha_* methods; replace
this hardcoded sequence with Bangladesh-specific holiday-window logic: create or
call a lookup (e.g., get_bangladesh_eid_windows(year) or a per-year mapping)
that returns the exact date ranges for Eid al-Fitr and Eid al-Adha for the given
year and iterate over those ranges to add each holiday date via the existing add
methods (or a single generic add-day helper), ensuring that for years where the
window is shorter/longer or shifted you only add the official dates rather than
always three consecutive days.
- Around line 166-174: Update the JUMATUL_WIDA data so 2025 is not suppressed:
set JUMATUL_WIDA_DATES[2025] = (MAR, 28) and add 2025 to
JUMATUL_WIDA_DATES_CONFIRMED_YEARS (update the tuple currently containing (2022,
2026)). Ensure you use the existing MAR constant and run/update
tests/localization that reference JUMATUL_WIDA_DATES and
JUMATUL_WIDA_DATES_CONFIRMED_YEARS.
In `@tests/countries/test_bangladesh.py`:
- Around line 65-135: Replace the broad positive assertions that use
self.no_estimated_holidays with a confined verified-year window and explicitly
assert absence outside it: in test_ashura_day, test_eid_e_milad_un_nabi,
test_eid_al_fitr, and test_eid_al_adha, change the second assertHolidayName call
that uses range(self.start_year, self.end_year) to only cover range(2022, 2026)
(the verified 2022–2025 window), and add complementary negative checks for years
before 2022 and after 2025 (e.g., use the test helper that asserts a holiday
name is not present) so the no-estimated fixture only applies to the confirmed
years; references: self.no_estimated_holidays, assertHolidayName,
test_ashura_day, test_eid_e_milad_un_nabi, test_eid_al_fitr, test_eid_al_adha.
🪄 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: 656af20b-f916-4519-abc9-d219ce800c44
📒 Files selected for processing (5)
holidays/countries/bangladesh.pyholidays/locale/ar/LC_MESSAGES/BD.poholidays/locale/bn/LC_MESSAGES/BD.poholidays/locale/en_US/LC_MESSAGES/BD.potests/countries/test_bangladesh.py
Co-authored-by: ~Jhellico <KJhellico@users.noreply.github.com> Signed-off-by: Shaon Ahamed <shaonahmed8788@gmail.com>
PPsyrius
left a comment
There was a problem hiding this comment.
Aside from this and previous @KJhellico on en_US l10n standardisation for US holidays - the rest are LGTM
IMO if you want to preserve local holiday names as well, you might want to add a separate en_BD l10n.
Co-authored-by: Panpakorn Siripanich <19505219+PPsyrius@users.noreply.github.com> Signed-off-by: Shaon Ahamed <shaonahmed8788@gmail.com>
Co-authored-by: Panpakorn Siripanich <19505219+PPsyrius@users.noreply.github.com> Signed-off-by: Shaon Ahamed <shaonahmed8788@gmail.com>
Co-authored-by: Panpakorn Siripanich <19505219+PPsyrius@users.noreply.github.com> Signed-off-by: Shaon Ahamed <shaonahmed8788@gmail.com>
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@scripts/generate_site_assets.py`:
- Line 127: The language display name for the "en_BD" entry in the site asset
map is misspelled ("Bangladehs"); update the value for the "en_BD" key in
generate_site_assets.py to "English (Bangladesh)" so the generated site assets
show the correct country name.
🪄 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: b75c646b-b472-4551-8c3e-f207b4add8fc
📒 Files selected for processing (4)
README.mdholidays/countries/bangladesh.pyholidays/locale/en_BD/LC_MESSAGES/BD.poscripts/generate_site_assets.py
Signed-off-by: Shaon Ahamed <shaonahmed8788@gmail.com>
|
Is everything correct now, or is there still something left to fix? |
|
Co-authored-by: ~Jhellico <KJhellico@users.noreply.github.com> Signed-off-by: Shaon Ahamed <shaonahmed8788@gmail.com>
Co-authored-by: ~Jhellico <KJhellico@users.noreply.github.com> Signed-off-by: Shaon Ahamed <shaonahmed8788@gmail.com>
|
Done✅ |
|
Proposed change
I have added Islamic holidays for Bangladesh.
Type of change
holidaysfunctionality in general)Checklist
make checklocally; all checks and tests passed.