Skip to content

Conversation

@PPsyrius
Copy link
Collaborator

@PPsyrius PPsyrius commented Oct 15, 2025

Proposed change

  • Add Thai (th) l10n for the Netherlands holidays.
  • Move start_year to 1966 instead (the year in which the Algemene termijnenwet (Atw) became effective for full year).
  • Refactor test cases to modern library-wide standards.

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

@coderabbitai
Copy link
Contributor

coderabbitai bot commented Oct 15, 2025

Summary by CodeRabbit

  • New Features
    • Added Thai (th) localization for Netherlands holidays.
    • Introduced Good Friday as a standard public holiday.
  • Changes
    • Updated King’s Day / Queen’s Day handling to reflect historical rules.
    • Adjusted Liberation Day observance years and occurrences.
    • Netherlands holiday coverage now starts from 1966.
  • Bug Fixes
    • Removed duplicate Good Friday translations and corrected placement across locales.
  • Documentation
    • Updated README to list Thai among supported languages for the Netherlands.
  • Tests
    • Reworked tests to use year ranges (1966–2050) and added locale coverage (th, uk, en).

Walkthrough

Netherlands holiday rules were revised (start year, Good Friday, King/Queen’s Day movement, Liberation Day cycle), Thai localization for NL was added, README language list updated, NL snapshot data refreshed, and Netherlands tests were expanded and refactored.

Changes

Cohort / File(s) Summary
Netherlands rules
holidays/countries/netherlands.py
Reworked holiday logic: updated start_year, added Good Friday as a standard holiday, unified King’s/Queen’s Day handling with year-based move rules, applied Bevrijdingsdag (Liberation Day) 5‑year cycle and adjusted thresholds, added Atw/CAO notes, and expanded supported_languages to include th.
Localizations (NL)
holidays/locale/en_US/LC_MESSAGES/NL.po, holidays/locale/fy/LC_MESSAGES/NL.po, holidays/locale/nl/LC_MESSAGES/NL.po, holidays/locale/th/LC_MESSAGES/NL.po, holidays/locale/uk/LC_MESSAGES/NL.po
Bumped Project-Id-Version headers; added/moved Goede Vrijdag (Good Friday) entries; removed duplicate blocks; added new Thai NL.po translations.
Snapshots
snapshots/countries/NL_COMMON.json
Updated static NL snapshot data: removed older pre-1950s–1960s blocks and added/adjusted Liberation Day (05-05) entries across multiple years and combined labels.
Tests
tests/countries/test_netherlands.py
Converted many year-specific tests into range-based assertions (spanning mid-1960s onward), added checks for Good Friday and other holidays across ranges, and expanded locale checks (including Thai).
Docs
README.md
Added th to the Netherlands supported languages in the country table.

Estimated code review effort

🎯 4 (Complex) | ⏱️ ~75 minutes

Suggested reviewers

  • arkid15r
  • KJhellico

Pre-merge checks and finishing touches

❌ Failed checks (1 warning)
Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. You can run @coderabbitai generate docstrings to improve docstring coverage.
✅ Passed checks (2 passed)
Check name Status Explanation
Title Check ✅ Passed The title directly references the two primary changes in the pull request by indicating the addition of Thai localization and the refactoring of test cases, using clear and concise phrasing that makes it easy for a reviewer to grasp the core updates.
Description Check ✅ Passed The pull request description clearly outlines the addition of Thai localization for Netherlands holidays, the adjustment of the start_year setting, and the test refactoring, all of which directly correspond to the detailed changes in the code, translation files, and tests. It is specific and relevant to the changeset provided.
✨ Finishing touches
  • 📝 Generate docstrings
🧪 Generate unit tests (beta)
  • 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

Comment @coderabbitai help to get the list of available commands and usage tips.

@codecov
Copy link

codecov bot commented Oct 15, 2025

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 100.00%. Comparing base (81da7b7) to head (9f8d9bd).
⚠️ Report is 3 commits behind head on dev.

Additional details and impacted files
@@            Coverage Diff            @@
##               dev     #3006   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files          306       306           
  Lines        18076     18069    -7     
  Branches      2334      2331    -3     
=========================================
- Hits         18076     18069    -7     

☔ 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

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 2

📜 Review details

Configuration used: Path: .coderabbit.yaml

Review profile: ASSERTIVE

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 4e7f667 and ff2b59b.

📒 Files selected for processing (9)
  • README.md (1 hunks)
  • holidays/countries/netherlands.py (3 hunks)
  • holidays/locale/en_US/LC_MESSAGES/NL.po (2 hunks)
  • holidays/locale/fy/LC_MESSAGES/NL.po (2 hunks)
  • holidays/locale/nl/LC_MESSAGES/NL.po (2 hunks)
  • holidays/locale/th/LC_MESSAGES/NL.po (1 hunks)
  • holidays/locale/uk/LC_MESSAGES/NL.po (2 hunks)
  • snapshots/countries/NL_COMMON.json (7 hunks)
  • tests/countries/test_netherlands.py (2 hunks)
🧰 Additional context used
🧠 Learnings (19)
📚 Learning: 2025-05-06T15:25:44.333Z
Learnt from: KJhellico
PR: vacanza/holidays#2530
File: holidays/locale/ca/LC_MESSAGES/AD.po:31-40
Timestamp: 2025-05-06T15:25:44.333Z
Learning: In the Holidays project, msgid fields in localization files contain strings in the entity's default language (as defined by default_language attribute), not English source strings as in standard gettext implementations.

Applied to files:

  • holidays/locale/en_US/LC_MESSAGES/NL.po
  • holidays/locale/fy/LC_MESSAGES/NL.po
  • holidays/locale/uk/LC_MESSAGES/NL.po
  • holidays/locale/nl/LC_MESSAGES/NL.po
📚 Learning: 2025-03-05T17:51:00.633Z
Learnt from: KJhellico
PR: vacanza/holidays#2259
File: holidays/locale/en_IN/LC_MESSAGES/IN.po:30-299
Timestamp: 2025-03-05T17:51:00.633Z
Learning: In the Holidays project, .po files for a country's default locale use empty msgstr fields as a standard convention.

Applied to files:

  • holidays/locale/en_US/LC_MESSAGES/NL.po
  • holidays/locale/fy/LC_MESSAGES/NL.po
  • holidays/locale/uk/LC_MESSAGES/NL.po
  • holidays/locale/nl/LC_MESSAGES/NL.po
📚 Learning: 2025-06-28T10:39:19.185Z
Learnt from: KJhellico
PR: vacanza/holidays#2684
File: holidays/locale/it/LC_MESSAGES/SM.po:13-13
Timestamp: 2025-06-28T10:39:19.185Z
Learning: In the holidays project, .po file header comments use the format "# [Country] holidays." for default language files (without trailing hash) and "# [Country] holidays [locale] localization." for non-default language files (also without trailing hash).

Applied to files:

  • holidays/locale/en_US/LC_MESSAGES/NL.po
  • holidays/locale/fy/LC_MESSAGES/NL.po
  • holidays/locale/uk/LC_MESSAGES/NL.po
  • holidays/locale/nl/LC_MESSAGES/NL.po
📚 Learning: 2025-06-25T10:09:29.029Z
Learnt from: PPsyrius
PR: vacanza/holidays#2676
File: holidays/locale/ar/LC_MESSAGES/EG.po:0-0
Timestamp: 2025-06-25T10:09:29.029Z
Learning: In the holidays library, msgstr fields can be left empty for source/default_language files when using Lingva, the localization tool used by the project.

Applied to files:

  • holidays/locale/en_US/LC_MESSAGES/NL.po
  • holidays/locale/fy/LC_MESSAGES/NL.po
  • holidays/locale/uk/LC_MESSAGES/NL.po
  • holidays/locale/nl/LC_MESSAGES/NL.po
📚 Learning: 2025-09-26T13:58:49.363Z
Learnt from: KJhellico
PR: vacanza/holidays#2960
File: holidays/locale/ca/LC_MESSAGES/ES.po:148-151
Timestamp: 2025-09-26T13:58:49.363Z
Learning: In the holidays project, when translating holiday names in .po files, the msgstr should be a faithful translation of the original msgid (in the country's default language), not influenced by English commentary or common en_US names. For example, "Día de la Pascua Granada" (Spanish) correctly translates to "Dia de la Pasqua Granada" (Catalan), even though the English comment refers to it as "Whit Monday".

Applied to files:

  • holidays/locale/en_US/LC_MESSAGES/NL.po
  • holidays/locale/fy/LC_MESSAGES/NL.po
  • holidays/locale/uk/LC_MESSAGES/NL.po
  • holidays/locale/nl/LC_MESSAGES/NL.po
📚 Learning: 2025-06-25T14:08:09.323Z
Learnt from: KJhellico
PR: vacanza/holidays#2651
File: holidays/locale/pap_BQ/LC_MESSAGES/BQ.po:70-72
Timestamp: 2025-06-25T14:08:09.323Z
Learning: The Papiamento translation "Di dos dia di Pasku" for "Tweede Kerstdag" (Boxing Day) in holidays/locale/pap_BQ/LC_MESSAGES/BQ.po comes from an authoritative source, despite the literal meaning potentially seeming to refer to Easter rather than Christmas.

Applied to files:

  • holidays/locale/en_US/LC_MESSAGES/NL.po
  • holidays/locale/fy/LC_MESSAGES/NL.po
  • holidays/locale/uk/LC_MESSAGES/NL.po
  • holidays/locale/nl/LC_MESSAGES/NL.po
📚 Learning: 2025-09-20T12:21:50.877Z
Learnt from: PPsyrius
PR: vacanza/holidays#2881
File: tests/countries/test_belgium.py:28-30
Timestamp: 2025-09-20T12:21:50.877Z
Learning: Belgium holidays implementation currently lacks a start_year attribute. In tests/countries/test_belgium.py, do not suggest adding test_no_holidays methods that rely on start_year until the start_year attribute is introduced to Belgium's holiday implementation.

Applied to files:

  • holidays/countries/netherlands.py
  • tests/countries/test_netherlands.py
📚 Learning: 2025-04-05T04:47:27.213Z
Learnt from: PPsyrius
PR: vacanza/holidays#2416
File: tests/countries/test_turkmenistan.py:52-64
Timestamp: 2025-04-05T04:47:27.213Z
Learning: For holiday tests in the vacanza/holidays project, structure tests by individual holidays rather than by years. Each test method should focus on a specific holiday and test it across multiple years (from start_year through 2050) using helper methods like `assertHolidayName`. For fixed holidays, use generators like `(f"{year}-01-01" for year in range(1991, 2051))`. For movable holidays, specify individual dates for specific years followed by a range check.

Applied to files:

  • tests/countries/test_netherlands.py
📚 Learning: 2025-05-06T21:07:11.577Z
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.

Applied to files:

  • tests/countries/test_netherlands.py
📚 Learning: 2025-04-05T06:49:06.217Z
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.

Applied to files:

  • tests/countries/test_netherlands.py
📚 Learning: 2025-09-18T03:19:23.722Z
Learnt from: PPsyrius
PR: vacanza/holidays#2881
File: tests/countries/test_algeria.py:28-30
Timestamp: 2025-09-18T03:19:23.722Z
Learning: In the vacanza/holidays project, tests now use self.start_year and self.end_year from the TestCase class instead of country-specific aliases (like DZ.start_year) for start_year and end_year references. This approach provides the test framework with better control over test year ranges rather than being tied to specific country start years.

Applied to files:

  • tests/countries/test_netherlands.py
📚 Learning: 2025-07-24T15:21:31.632Z
Learnt from: PPsyrius
PR: vacanza/holidays#2750
File: tests/countries/test_germany.py:46-46
Timestamp: 2025-07-24T15:21:31.632Z
Learning: In the holidays project test files, the standard method name for testing the absence of holidays is `test_no_holidays`, not more descriptive names like `test_no_holidays_before_1990`. This is a consistent naming convention across country test files like France and Germany.

Applied to files:

  • tests/countries/test_netherlands.py
📚 Learning: 2025-09-28T05:42:12.777Z
Learnt from: PPsyrius
PR: vacanza/holidays#2881
File: tests/countries/test_indonesia.py:221-223
Timestamp: 2025-09-28T05:42:12.777Z
Learning: In tests/countries/test_indonesia.py, the manual set inclusion checks using get_named and years_found for Lunar New Year (test_lunar_new_year) and Vesak Day (test_vesak_day) are intentional and should remain until both holidays get their own `{insert}_no_estimated` flags implemented, rather than using standard harness assertions like assertHolidayName/assertNoHolidayName.

Applied to files:

  • tests/countries/test_netherlands.py
📚 Learning: 2025-05-09T18:36:09.607Z
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.

Applied to files:

  • tests/countries/test_netherlands.py
📚 Learning: 2025-09-14T04:41:10.139Z
Learnt from: PPsyrius
PR: vacanza/holidays#2881
File: tests/countries/test_south_africa.py:22-22
Timestamp: 2025-09-14T04:41:10.139Z
Learning: South Africa's observed holiday system only started in 1995, so in tests/countries/test_south_africa.py, using years_non_observed=range(1995, 2050) is intentional to limit testing to years where observed holidays actually exist, improving both correctness and performance.

Applied to files:

  • tests/countries/test_netherlands.py
📚 Learning: 2025-09-10T14:35:54.603Z
Learnt from: PPsyrius
PR: vacanza/holidays#2881
File: tests/countries/test_brazil.py:28-30
Timestamp: 2025-09-10T14:35:54.603Z
Learning: In the holidays project, the test_no_holidays method should test ALL supported_categories (via categories=CountryClass.supported_categories) rather than just the default PUBLIC category, to ensure comprehensive validation that no holidays exist before start_year across any supported category including OPTIONAL and subdivision categories.

Applied to files:

  • tests/countries/test_netherlands.py
📚 Learning: 2025-09-20T12:24:28.864Z
Learnt from: PPsyrius
PR: vacanza/holidays#2881
File: tests/countries/test_curacao.py:29-29
Timestamp: 2025-09-20T12:24:28.864Z
Learning: For Curacao in the holidays library, PUBLIC and HALF_DAY categories have different start years - PUBLIC holidays begin at the country's start_year while HALF_DAY holidays begin in 2010. When testing no_holidays methods, each category should be tested separately with appropriate pre-start years rather than using a unified supported_categories approach.

Applied to files:

  • tests/countries/test_netherlands.py
📚 Learning: 2025-09-14T16:03:13.558Z
Learnt from: PPsyrius
PR: vacanza/holidays#2881
File: tests/countries/test_barbados.py:21-23
Timestamp: 2025-09-14T16:03:13.558Z
Learning: In tests/countries/test_barbados.py, using years_non_observed=range(2000, 2024) is intentional because all observed holiday examples fall within 2001-2023, making this range appropriate for limiting testing to years where observed holidays actually exist in the test data.

Applied to files:

  • tests/countries/test_netherlands.py
📚 Learning: 2025-06-25T20:55:00.642Z
Learnt from: KJhellico
PR: vacanza/holidays#2651
File: holidays/locale/nl/LC_MESSAGES/BQ.po:29-95
Timestamp: 2025-06-25T20:55:00.642Z
Learning: In the holidays library, Dutch locale files (.po) with `X-Source-Language: nl` should have empty msgstr entries when the target language is also Dutch. The library uses fallback=True with gettext, which returns the original msgid when msgstr is empty. This is the correct pattern for native language files and does not cause blank holiday names.

Applied to files:

  • holidays/locale/th/LC_MESSAGES/NL.po
🧬 Code graph analysis (2)
holidays/countries/netherlands.py (3)
holidays/groups/international.py (1)
  • _add_new_years_day (142-150)
holidays/groups/christian.py (3)
  • _add_good_friday (344-353)
  • _add_easter_sunday (286-295)
  • _add_easter_monday (275-284)
holidays/observed_holiday_base.py (1)
  • _move_holiday (199-211)
tests/countries/test_netherlands.py (3)
tests/countries/test_slovakia.py (1)
  • setUpClass (22-25)
tests/common.py (6)
  • setUpClass (35-56)
  • assertAliases (124-133)
  • assertNoHolidays (324-326)
  • assertHolidayName (198-202)
  • assertNoHolidayName (305-307)
  • assertLocalizedHolidays (359-370)
holidays/countries/netherlands.py (3)
  • Netherlands (20-98)
  • NL (101-102)
  • NLD (105-106)
🔇 Additional comments (15)
holidays/locale/nl/LC_MESSAGES/NL.po (2)

17-17: Header bump OK.

Version to 0.83 looks good; nl default-locale keeps empty msgstr entries.


34-37: Good Friday entry correctly added and positioned.

Placement before Easter Sunday matches convention; empty msgstr is correct for nl default locale.

holidays/locale/fy/LC_MESSAGES/NL.po (2)

17-17: Header bump OK.

Project-Id-Version updated to 0.83; metadata consistent.


33-36: Good Friday translation LGTM.

"Goede Vrijdag" → "Goedfreed" is accurate; ordering is consistent with other locales.

holidays/locale/uk/LC_MESSAGES/NL.po (2)

17-17: Header bump OK.

Metadata update to 0.83 is fine.


35-37: Good Friday translation LGTM.

"Страсна пʼятниця" is correct; order consistent with other locales.

holidays/countries/netherlands.py (3)

39-41: Start year and language set are appropriate.

Atw alignment with start_year=1965 and adding "th" to supported_languages look good.


50-57: New Year gating and Good Friday inclusion make sense.

  • New Year only from 1966 (since 1965-01-01 precedes Atw) is correct.
  • Good Friday added unconditionally within start_year scope is fine.

64-71: Weekend move rule for King’s/Queen’s Day looks correct.

SUN→prev Sat from 1980; otherwise SUN→next Mon matches historical handling.

holidays/locale/th/LC_MESSAGES/NL.po (1)

54-57: Fix Thai translation for Liberation Day.

"วันประกาศอิสรภาพ" means “Independence Day”; replace with the standard Thai term “วันปลดปล่อย”.

Apply this diff:

 msgid "Bevrijdingsdag"
-msgstr "วันประกาศอิสรภาพ"
+msgstr "วันปลดปล่อย"
README.md (1)

1225-1225: NL supported languages update looks correct (added th).
Consistent with Netherlands.supported_languages.

holidays/locale/en_US/LC_MESSAGES/NL.po (1)

17-27: Header bump and Good Friday translation placement look good.

  • en_US msgstr entries correctly translate nl msgids.
  • Good Friday entry is present once and positioned logically before Easter.

Based on learnings

Also applies to: 34-37

snapshots/countries/NL_COMMON.json (1)

2-12: Snapshot aligns with new NL rules (start_year=1965, Bevrijdingsdag cadence, merged names when overlapping).
Looks consistent with tests and implementation.

If your snapshot generator intentionally includes OPTIONAL along with PUBLIC in COMMON snapshots, no action needed. Otherwise, confirm Bevrijdingsdag presence matches intended category inclusion policy for snapshots.

Also applies to: 180-206, 428-436, 552-556

tests/countries/test_netherlands.py (2)

23-26: Class setup is solid.
Year range and OPTIONAL instance construction follow project patterns.


30-32: Good coverage for “no holidays” pre-start_year across all categories.
Matches project guidance to validate supported_categories, not just PUBLIC.

Co-authored-by: ~Jhellico <KJhellico@users.noreply.github.com>
@PPsyrius PPsyrius requested a review from KJhellico October 15, 2025 11:00
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!

@sonarqubecloud
Copy link

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 Oct 16, 2025
Merged via the queue into vacanza:dev with commit 04c2dae Oct 16, 2025
36 checks passed
@PPsyrius PPsyrius deleted the nl_th_l10n branch October 17, 2025 01:55
@KJhellico KJhellico mentioned this pull request Oct 20, 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.

3 participants