Add SCHOOL category and subdivision hooks for Belgium#3195
Add SCHOOL category and subdivision hooks for Belgium#3195Upanshi-Mittal wants to merge 8 commits intovacanza:devfrom
Conversation
Upanshi-Mittal
requested review from
KJhellico,
PPsyrius and
arkid15r
as code owners
WalkthroughAdds Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 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)
Tip Try Coding Plans. Let us write the prompt for your AI agent so you can ship faster (with fewer bugs). 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 |
|
@Upanshi-Mittal, what review are you waiting for on this draft? %) |
|
@KJhellico this PR is intentionally limited to structure/scaffolding only. The idea is to first align on the approach before following up with a separate PR that implements the real holiday rules and data. |
|
I don't see the point in separate PR. Just fill this one with code. |
|
Got it. I’ll extend this PR and add the actual Belgian school holiday logic here instead of creating a follow-up PR. |
|
This PR now includes full implementation of Belgian school holidays for VLG, WBR and GER subdivisions along with documentation examples. |
holidays/countries/belgium.py
Outdated
| * <https://web.archive.org/web/20250331001402/https://www.belgium.be/nl/over_belgie/land/belgie_in_een_notendop/feestdagen> | ||
| * <https://nl.wikipedia.org/wiki/Feestdagen_in_België> | ||
| * <https://web.archive.org/web/20240816004739/https://www.nbb.be/en/about-national-bank/national-bank-belgium/public-holidays> | ||
| def nth_weekday_of_month(year, month, weekday, n): |
There was a problem hiding this comment.
As I already noted,
Make sure you have an understanding of the established code style and approaches used in project.
The code currently doesn't meet the project requirements at all.
|
@KJhellico Thanks for pointing that out. I realize now that I implemented the logic without properly following the existing country implementations and code style. I’ll refactor this PR to align with the established patterns used in the project, remove unnecessary custom helpers, and simplify the holiday calculations. I’ll push an updated version shortly. |
|
@KJhellico one small question before I go ahead and refactor more. Paasvakantie is the part that is confusing me the most. I tried to derive it only from Easter-based rules, but for multiple years (like 2026, 2027, 2029) it doesn’t match the official Flemish school calendars. So I’m not sure what approach is preferred here. Should I:
I just want to make sure I’m following the project’s direction before continuing. |
Try to define it in the simplest possible rule (rules) with the fewest exceptions. |
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
|
KJhellico
left a comment
KJhellico
left a comment
There was a problem hiding this comment.
Since pre-commit check has already passed, we can take a look at the code. :) I also recommend starting to write tests.
|
@Upanshi-Mittal, are you planning to complete this PR? |
|
@KJhellico |
…use gregorian helpers
Upanshi-Mittal
force-pushed
the
belgium-school-holidays
branch
from
a33785c to
09a7fd2
Compare
There was a problem hiding this comment.
Actionable comments posted: 6
🤖 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/belgium.py`:
- Around line 305-310: The comment above the Easter holiday logic is incorrect
and describes VLG rules instead of the actual implementation; update the comment
to accurately describe what the code does (compute paas_start via
_timedelta(easter, 1) — the Monday after Easter — and add a 13-day multi-day
holiday via self._add_multiday_holiday(paas_start, 13,
name=tr("Paasvakantie"))), or if the intended rule is the complex VLG logic then
implement that logic in place of the simple _timedelta call; adjust the comment
accordingly and keep references to paas_start, _timedelta, and
_add_multiday_holiday consistent.
- Line 181: The school-holiday calls to _add_ascension_thursday use
tr("Hemelvaart") which doesn't match the public-holiday msgid; update the
school-holiday invocations (the three places that call _add_ascension_thursday)
to use tr("O. L. H. Hemelvaart") so they match the public holiday usage and
existing BE.po entries (alternatively, add "Hemelvaart" as a msgid to all BE.po
files, but prefer aligning code to "O. L. H. Hemelvaart" for consistency).
- Line 146: Replace the magic weekday literal with the exported constant: in the
call to _get_nth_weekday_from that currently uses 0 as the weekday argument
(start = _get_nth_weekday_from(..., 0, christmas)), use the MON constant instead
so it reads start = _get_nth_weekday_from(..., MON, christmas); this keeps usage
consistent with other calls like _get_nth_weekday_from(-1, MON, krokus_raw) and
matches the imported MON from holidays.calendars.gregorian.
In `@holidays/locale/en_US/LC_MESSAGES/BE.po`:
- Around line 119-121: The translator comment for the "Heiligavond" entry lacks
a trailing period; update the comment line that begins with "#. Christmas Eve"
(the entry with msgid "Heiligavond" and msgstr "Christmas Eve") to end with a
period so it reads "#. Christmas Eve." to match en_US comment style conventions.
- Around line 91-93: The translator comment for the entry with msgid
"Kerstvakantie" and msgstr "Christmas Holidays" is singular ("Christmas
Holiday."); update the comment to use the plural form to match msgstr (e.g.,
"Christmas Holidays.") or change msgstr to singular to match the comment—edit
the comment line above msgid so the comment text and the msgstr agree.
- Around line 95-97: Update the translator comment to use the plural form to
match the msgstr; change the comment string that currently reads "Carnival
Holiday." to "Carnival Holidays." so the translator note for msgid
"Krokusvakantie" / msgstr "Carnival Holidays" correctly reflects the plural.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 1af5dc30-ef79-4759-8a7b-bfb37cc6d6cc
📒 Files selected for processing (6)
docs/holiday_categories.mdholidays/countries/belgium.pyholidays/locale/de/LC_MESSAGES/BE.poholidays/locale/en_US/LC_MESSAGES/BE.poholidays/locale/fr/LC_MESSAGES/BE.poholidays/locale/uk/LC_MESSAGES/BE.po
KJhellico
left a comment
KJhellico
left a comment
There was a problem hiding this comment.
Few suggestions on Flemish holidays. The rest of methods also requires similar changes in code style.
holidays/countries/belgium.py
Outdated
| # Carnival holidays. | ||
| krokus_raw = _timedelta(easter, days=-47) | ||
| krokus_start = _get_nth_weekday_from(-1, MON, krokus_raw) | ||
| self._add_multiday_holiday(krokus_start, 7, name=tr("Krokusvakantie")) |
There was a problem hiding this comment.
| # Carnival holidays. | |
| krokus_raw = _timedelta(easter, days=-47) | |
| krokus_start = _get_nth_weekday_from(-1, MON, krokus_raw) | |
| self._add_multiday_holiday(krokus_start, 7, name=tr("Krokusvakantie")) | |
| # Carnival Holidays: Carnival Monday (Easter - 48 days) → 1 week. | |
| # Carnival Holidays. | |
| self._add_multiday_holiday(_timedelta(easter, -48), 7, name=tr("Krokusvakantie")) |
| elif easter.day >= 16: | ||
| easter_start = _timedelta(easter, -13) | ||
| easter_duration = 15 | ||
| self._add_multiday_holiday(easter_start, easter_duration, name=tr("Paasvakantie")) |
There was a problem hiding this comment.
| self._add_multiday_holiday(easter_start, easter_duration, name=tr("Paasvakantie")) | |
| # Easter Holidays. | |
| self._add_multiday_holiday(easter_start, easter_duration, name=tr("Paasvakantie")) |
holidays/countries/belgium.py
Outdated
| # Friday after Ascension. | ||
| ascension_friday = _timedelta(easter, days=40) | ||
| self._add_holiday(tr("Vrijdag na O. L. H. Hemelvaart"), ascension_friday) |
There was a problem hiding this comment.
| # Friday after Ascension. | |
| ascension_friday = _timedelta(easter, days=40) | |
| self._add_holiday(tr("Vrijdag na O. L. H. Hemelvaart"), ascension_friday) | |
| # Friday after Ascension Day. | |
| self._add_holiday_40_days_past_easter(tr("Vrijdag na O. L. H. Hemelvaart")) |
holidays/countries/belgium.py
Outdated
| ascension_friday = _timedelta(easter, days=40) | ||
| self._add_holiday(tr("Vrijdag na O. L. H. Hemelvaart"), ascension_friday) | ||
|
|
||
| # Whit Monday / Pentecost Monday. |
There was a problem hiding this comment.
| # Whit Monday / Pentecost Monday. | |
| # Whit Monday. |
holidays/countries/belgium.py
Outdated
| # Summer holidays. | ||
| start = date(year, JUL, 1) | ||
| end = date(year, SEP, 1) | ||
| self._add_multiday_holiday(start, (end - start).days, name=tr("Zomervakantie")) |
There was a problem hiding this comment.
| # Summer holidays. | |
| start = date(year, JUL, 1) | |
| end = date(year, SEP, 1) | |
| self._add_multiday_holiday(start, (end - start).days, name=tr("Zomervakantie")) | |
| # Summer Holidays: From Jul 1 to Aug 31 (fixed period). | |
| # Summer Holidays. | |
| self._add_multiday_holiday(date(self._year, JUL, 1), 62, name=tr("Zomervakantie")) |
holidays/countries/belgium.py
Outdated
| # Autumn holidays (week of 1 November). | ||
| nov_1 = date(year, NOV, 1) | ||
| start = _get_nth_weekday_from(-1 if nov_1.weekday() <= 5 else 1, 0, nov_1) | ||
| self._add_multiday_holiday(start, 7, name=tr("Herfstvakantie")) |
There was a problem hiding this comment.
| # Autumn holidays (week of 1 November). | |
| nov_1 = date(year, NOV, 1) | |
| start = _get_nth_weekday_from(-1 if nov_1.weekday() <= 5 else 1, 0, nov_1) | |
| self._add_multiday_holiday(start, 7, name=tr("Herfstvakantie")) | |
| # Autumn Holidays: Monday of the week of Nov 1 → 1 week. | |
| # If Nov 1 is on Sun, start from Nov 2. | |
| nov_1 = date(self._year, NOV, 1) | |
| autumn_start = _get_nth_weekday_from(1 if self._is_sunday(nov_1) else -1, MON, nov_1) | |
| # Autumn Holidays. | |
| self._add_multiday_holiday(autumn_start, 7, name=tr("Herfstvakantie")) |
holidays/countries/belgium.py
Outdated
| start = _get_nth_weekday_from(-1 if nov_1.weekday() <= 5 else 1, 0, nov_1) | ||
| self._add_multiday_holiday(start, 7, name=tr("Herfstvakantie")) | ||
|
|
||
| # Armistice Day (End of World War I). |
There was a problem hiding this comment.
| # Armistice Day (End of World War I). | |
| # Armistice Day. |
|
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
holidays/countries/belgium.py (1)
251-252:⚠️ Potential issue | 🟡 MinorKeep the l10n comments in en_US spelling.
These adjacent comments are part of the localization source. Using
Labour Day.here will drift the generated catalogs away from the existing en_US wording. Change both toLabor Day..Based on learnings, message comments should always use en_US spelling conventions.
Also applies to: 320-321
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@holidays/countries/belgium.py` around lines 251 - 252, The comment above the localization call uses en_GB spelling "Labour Day."; update the comment text to en_US spelling "Labor Day." for the instance adjacent to self._add_labor_day(tr("Dag van de Arbeid")) and the other duplicate occurrence near the second self._add_labor_day call (lines noted in the review) so the l10n source comments remain consistent with en_US conventions.
🤖 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/belgium.py`:
- Around line 238-240: The current code calls tr("Paasvakantie") in the Belgium
holiday block (see _get_nth_weekday_of_month and self._add_multiday_holiday use)
but that msgid is shared with the general Easter Holidays; create a distinct
Dutch msgid for the WBR spring break (e.g. replace tr("Paasvakantie") with
tr("WBR Paasvakantie") in the self._add_multiday_holiday call) and add
corresponding entries to the BE .po files (and other locale .po files as needed)
so the WBR term can be translated independently from the existing Easter
Holidays translation.
---
Duplicate comments:
In `@holidays/countries/belgium.py`:
- Around line 251-252: The comment above the localization call uses en_GB
spelling "Labour Day."; update the comment text to en_US spelling "Labor Day."
for the instance adjacent to self._add_labor_day(tr("Dag van de Arbeid")) and
the other duplicate occurrence near the second self._add_labor_day call (lines
noted in the review) so the l10n source comments remain consistent with en_US
conventions.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: cebce084-23f8-4974-948b-c834e21ce2ac
📒 Files selected for processing (2)
holidays/countries/belgium.pyholidays/locale/en_US/LC_MESSAGES/BE.po
| # Spring Holidays. | ||
| spring_start = _get_nth_weekday_of_month(4, MON, APR, year) | ||
| self._add_multiday_holiday(spring_start, 14, name=tr("Paasvakantie")) |
There was a problem hiding this comment.
Give WBR spring break its own source string.
tr("Paasvakantie") is already translated as "Easter Holidays" in holidays/locale/en_US/LC_MESSAGES/BE.po, so this subdivision can never surface "Spring Holidays" in non-Dutch locales. Use a distinct Dutch msgid for the WBR term and add matching entries to the BE .po files.
Based on learnings, msgid fields in localization files contain strings in the entity’s default language and msgstr should be a faithful translation of that msgid.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@holidays/countries/belgium.py` around lines 238 - 240, The current code calls
tr("Paasvakantie") in the Belgium holiday block (see _get_nth_weekday_of_month
and self._add_multiday_holiday use) but that msgid is shared with the general
Easter Holidays; create a distinct Dutch msgid for the WBR spring break (e.g.
replace tr("Paasvakantie") with tr("WBR Paasvakantie") in the
self._add_multiday_holiday call) and add corresponding entries to the BE .po
files (and other locale .po files as needed) so the WBR term can be translated
independently from the existing Easter Holidays translation.
There was a problem hiding this comment.
@KJhellico
Since Paasvakantie is already used for Easter holidays in other subdivisions, should WBR use a separate msgid (e.g. Lentevakantie) for Spring Holidays, or keep Paasvakantie for consistency?
There was a problem hiding this comment.
Seems like the humans are having a chat. I'll hop back into my burrow for now. If you need me again, just tag @coderabbitai in a new comment, and I'll come hopping out!
|
The duplication mainly comes from adding public holidays (e.g. Labor Day) inside each subdivision’s school holiday method. Since these are also public holidays, I wanted to confirm whether they should remain duplicated in the school methods or be handled only in |
| year = self._year | ||
| easter = self._easter_sunday | ||
|
|
||
| # Christmas holidays. |
There was a problem hiding this comment.
| # Christmas holidays. | |
| # Christmas Holidays. |
Pay closer attention to details, it's important.
And I find it useful to leave a short description of the rules for each vacation period before the corresponding block, as was done in your previous editions.
| prev_christmas = self._christmas_day.replace(year=self._year - 1) | ||
| prev_christmas_start = _get_nth_weekday_from( | ||
| 1 if self._is_weekend(prev_christmas) else -1, | ||
| MON, | ||
| prev_christmas, | ||
| ) | ||
| duration = 14 - (32 - prev_christmas_start.day) | ||
| self._add_multiday_holiday(date(self._year, JAN, 1), duration, name=name) |
There was a problem hiding this comment.
| prev_christmas = self._christmas_day.replace(year=self._year - 1) | |
| prev_christmas_start = _get_nth_weekday_from( | |
| 1 if self._is_weekend(prev_christmas) else -1, | |
| MON, | |
| prev_christmas, | |
| ) | |
| duration = 14 - (32 - prev_christmas_start.day) | |
| self._add_multiday_holiday(date(self._year, JAN, 1), duration, name=name) | |
| new_year = date(self._year, JAN, 1) | |
| christmas_last_part_duration = (4 - new_year.weekday()) % 7 + 3 | |
| self._add_multiday_holiday(new_year, christmas_last_part_duration, name=name) |
The logic is a little confusing, but it works. ;)
| # Carnival holidays. | ||
| carnival_raw = _timedelta(easter, days=-47) | ||
| carnival_start = _get_nth_weekday_from(-1, MON, carnival_raw) | ||
| self._add_multiday_holiday(carnival_start, 7, name=tr("Krokusvakantie")) |
There was a problem hiding this comment.
| # Carnival holidays. | |
| carnival_raw = _timedelta(easter, days=-47) | |
| carnival_start = _get_nth_weekday_from(-1, MON, carnival_raw) | |
| self._add_multiday_holiday(carnival_start, 7, name=tr("Krokusvakantie")) | |
| # Carnival Holidays: Carnival Monday (Easter - 48 days) → 1 week. | |
| # Carnival Holidays. | |
| self._add_multiday_holiday(_timedelta(easter, -48), 7, name=tr("Krokusvakantie")) |
You missed it.
| # Ascension Day. | ||
| self._add_ascension_thursday(tr("O. L. H. Hemelvaart")) | ||
|
|
||
| # Friday after Ascension. |
There was a problem hiding this comment.
| # Friday after Ascension. | |
| # Friday after Ascension Day. |
❗
| # Whit Monday. | ||
| self._add_whit_monday(tr("Pinkstermaandag")) | ||
|
|
||
| # Summer holidays. |
There was a problem hiding this comment.
| # Summer holidays. | |
| # Summer Holidays. |
| # Autumn holidays. | ||
| nov_1 = date(year, NOV, 1) | ||
| autumn_start = _get_nth_weekday_from(1 if self._is_sunday(nov_1) else -1, MON, nov_1) | ||
| self._add_multiday_holiday(autumn_start, 7, name=tr("Herfstvakantie")) |
There was a problem hiding this comment.
| self._add_multiday_holiday(autumn_start, 7, name=tr("Herfstvakantie")) | |
| # Autumn Holidays. | |
| self._add_multiday_holiday(autumn_start, 7, name=tr("Herfstvakantie")) |
All of these are l10n comments.
| # Armistice Day. | ||
| self._add_remembrance_day(tr("Wapenstilstand")) | ||
|
|
||
| def _populate_subdiv_wbr_school_holidays(self): |
There was a problem hiding this comment.
In 2022, the school year schedule in the French-speaking community has changed, which must be considered.
2 participants
Proposed change
This PR introduces initial infrastructure for Belgian school holidays.
It adds support for the
SCHOOLholiday category in Belgium and defines subdivision-specific hooks for the three Belgian school communities:French, Flemish, and German-speaking.
No placeholder or speculative dates are included; holidays with unclear or unstable definitions are intentionally left unimplemented.
This PR follows the guidance discussed in #2506 and prepares the codebase for future, data-backed implementations.
Type of change
holidaysfunctionality in general)Checklist
SCHOOLcategory initialization).