-
-
Notifications
You must be signed in to change notification settings - Fork 558
Add Svalbard and Jan Mayen holidays #2638
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
Summary by CodeRabbit
WalkthroughThis change introduces support for Svalbard and Jan Mayen (SJ) holidays as a new country in the holidays library. It adds a dedicated module, updates documentation, expands localization files, modifies the country registry, and provides comprehensive test and snapshot data for SJ and related Norwegian subdivisions. Changes
Assessment against linked issues
Assessment against linked issues: Out-of-scope changesNo out-of-scope changes detected. Possibly related PRs
Suggested labels
✨ Finishing Touches
🧪 Generate Unit Tests
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
Documentation and Community
|
|
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## dev #2638 +/- ##
=========================================
Coverage 100.00% 100.00%
=========================================
Files 232 233 +1
Lines 14739 14759 +20
Branches 2057 2057
=========================================
+ Hits 14739 14759 +20 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
There was a problem hiding this 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
🔭 Outside diff range comments (2)
snapshots/countries/NO_18.json (2)
1-1206
: Filename doesn't match PR scope
NO_18.json
represents the Nordland subdivision (ISO 3166-2:NO-18), but this PR is about Svalbard (NO-21) and Jan Mayen (NO-22). Please remove or rename it to the correct snapshots.
1-1206
: Huge duplication in snapshots
This file duplicates the entire Norway dataset, which bloats the repo. Consider generating snapshots programmatically or factoring out the common base.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (24)
README.md
(3 hunks)holidays/countries/__init__.py
(1 hunks)holidays/countries/norway.py
(1 hunks)holidays/countries/svalbard_and_jan_mayen.py
(1 hunks)holidays/locale/en_US/LC_MESSAGES/SJ.po
(1 hunks)holidays/locale/no/LC_MESSAGES/SJ.po
(1 hunks)holidays/locale/th/LC_MESSAGES/SJ.po
(1 hunks)holidays/locale/uk/LC_MESSAGES/SJ.po
(1 hunks)holidays/registry.py
(1 hunks)snapshots/countries/NO_03.json
(1 hunks)snapshots/countries/NO_11.json
(1 hunks)snapshots/countries/NO_15.json
(1 hunks)snapshots/countries/NO_18.json
(1 hunks)snapshots/countries/NO_21.json
(1 hunks)snapshots/countries/NO_22.json
(1 hunks)snapshots/countries/NO_30.json
(1 hunks)snapshots/countries/NO_34.json
(1 hunks)snapshots/countries/NO_38.json
(1 hunks)snapshots/countries/NO_42.json
(1 hunks)snapshots/countries/NO_46.json
(1 hunks)snapshots/countries/NO_50.json
(1 hunks)snapshots/countries/NO_54.json
(1 hunks)snapshots/countries/SJ_COMMON.json
(1 hunks)tests/countries/test_svalbard_and_jan_mayen.py
(1 hunks)
🧰 Additional context used
🧠 Learnings (2)
snapshots/countries/SJ_COMMON.json (1)
Learnt from: PPsyrius
PR: vacanza/holidays#2407
File: snapshots/countries/TL_COMMON.json:7-7
Timestamp: 2025-04-03T05:59:57.480Z
Learning: In the holidays project, snapshot files (like snapshots/countries/TL_COMMON.json) are auto-generated when running `make snapshot` and should not be manually edited. Semicolons (;) in holiday entries are used as separators when multiple holidays occur on the same date.
snapshots/countries/NO_03.json (1)
Learnt from: PPsyrius
PR: vacanza/holidays#2407
File: snapshots/countries/TL_COMMON.json:7-7
Timestamp: 2025-04-03T05:59:57.480Z
Learning: In the holidays project, snapshot files (like snapshots/countries/TL_COMMON.json) are auto-generated when running `make snapshot` and should not be manually edited. Semicolons (;) in holiday entries are used as separators when multiple holidays occur on the same date.
🧬 Code Graph Analysis (1)
holidays/countries/__init__.py (1)
holidays/countries/svalbard_and_jan_mayen.py (4)
SvalbardAndJanMayen
(38-39)SJ
(42-43)SJM
(46-47)HolidaysSJ
(16-35)
🪛 Pylint (3.3.7)
holidays/countries/svalbard_and_jan_mayen.py
[convention] 1-1: Missing module docstring
(C0114)
[convention] 38-38: Missing class docstring
(C0115)
[convention] 42-42: Missing class docstring
(C0115)
[convention] 46-46: Missing class docstring
(C0115)
tests/countries/test_svalbard_and_jan_mayen.py
[convention] 1-1: Missing module docstring
(C0114)
[convention] 19-19: Missing class docstring
(C0115)
[warning] 21-21: Number of parameters was 4 in 'TestCase.setUpClass' and is now 1 in overriding 'TestSvalbardAndJanMayen.setUpClass' method
(W0221)
[convention] 24-24: Missing function or method docstring
(C0116)
[convention] 27-27: Missing function or method docstring
(C0116)
[convention] 43-43: Missing function or method docstring
(C0116)
[convention] 60-60: Missing function or method docstring
(C0116)
[convention] 77-77: Missing function or method docstring
(C0116)
⏰ Context from checks skipped due to timeout of 300000ms (3)
- GitHub Check: Test Python 3.13 on windows-latest
- GitHub Check: Test Python 3.10 on windows-latest
- GitHub Check: Test Python 3.9 on windows-latest
🔇 Additional comments (32)
snapshots/countries/NO_22.json (1)
1-1206
: Static snapshot approved.
This JSON is a read-only data resource mapping dates to holiday names; no executable logic to review.snapshots/countries/NO_30.json (1)
1-1206
: Static snapshot approved.
This JSON is a read-only data resource mapping dates to holiday names; no executable logic to review.holidays/registry.py (1)
183-183
: Registry entry added for Svalbard and Jan Mayen.
The new key"svalbard_and_jan_mayen"
aligns with the module import and class aliases.holidays/countries/__init__.py (1)
180-180
: Imported new country module for SJ.
The import ofSvalbardAndJanMayen, SJ, SJM, HolidaysSJ
follows alphabetical order and matches the registry.holidays/locale/uk/LC_MESSAGES/SJ.po (1)
1-29
: Ukrainian PO file scaffold added.
Header looks correct; translation entries can be populated in the next pass.holidays/locale/th/LC_MESSAGES/SJ.po (1)
1-29
: Approve: Thai PO skeleton.
The new Thai localization file contains correct headers and stub structure for Svalbard and Jan Mayen translations.README.md (3)
108-108
: Country count updated correctly.
The total supported country codes has been incremented to 189 to include SJ.
1270-1275
: New entry for Svalbard and Jan Mayen added.
The SJ table row aligns with the new country module and locales, listing correct code, aliases, and supported languages.
1041-1041
: Let's verify each code and name individually:#!/bin/bash # Check for subdivision codes and names in Norway class rg -n '21:' holidays/countries/norway.py rg -n '22:' holidays/countries/norway.py rg -n 'Svalbard' holidays/countries/norway.py rg -n 'Jan Mayen' holidays/countries/norway.pyholidays/locale/no/LC_MESSAGES/SJ.po (1)
1-29
: Approve: Norwegian PO skeleton.
All headers and metadata are correctly set for the Norwegian translation stub of SJ.holidays/locale/en_US/LC_MESSAGES/SJ.po (1)
1-29
: Approve: English PO skeleton.
The English (en_US) localization file stub is properly configured with the correct project metadata for SJ.snapshots/countries/NO_38.json (1)
1-14
: Approve: Snapshot data for county code 38.
The static JSON snapshot for Norway subdivision 38 includes comprehensive and consistent holiday mappings, matching the pattern of existing snapshot files.holidays/countries/norway.py (2)
37-51
: Subdivision metadata correctly added
The newsubdivisions
tuple cleanly lists all region codes, including Svalbard (21
) and Jan Mayen (22
). Good use of type annotation.
52-69
: Subdivision aliases mapping looks solid
Thesubdivisions_aliases
cover all human-friendly names and alternate spellings for Norwegian regions. Ensure keys match locale variations consistently.snapshots/countries/NO_03.json (1)
1-1205
: Auto-generated snapshot – skipping manual review
This file is generated viamake snapshot
; no hand edits expected.snapshots/countries/NO_42.json (1)
1-1206
: Auto-generated snapshot – skipping manual review
This file is generated viamake snapshot
; no hand edits expected.snapshots/countries/NO_50.json (1)
1-1205
: Static snapshot consistencyNice work — the JSON is well-formed (chronologically sorted keys, consistent indentation, no trailing commas) and mirrors the structure of existing Norway subdivision snapshots. Please verify that subdivision code
NO_50
(Svalbard and Jan Mayen) is registered inholidays/countries/norway.py
and referenced in your test suite.Run this to confirm:
#!/bin/bash # Verify snapshot usage for NO_50 rg -l '"NO_50"' -n . rg -l 'NO_50' -n holidays/countries/norway.pysnapshots/countries/NO_11.json (1)
1-1205
: Let’s list the repo root to locate the test directory and search for any references to our snapshot file:#!/bin/bash # Show top‐level directories ls -1 # Look for any references to the NO_11 snapshot in code/tests rg -n "NO_11.json" -n .holidays/countries/svalbard_and_jan_mayen.py (1)
38-47
: Nit: tiny docstrings keep alias classes discoverable.The three alias classes are fine, yet adding a one-liner avoids pylint C0115 and helps IDEs.
-class SvalbardAndJanMayen(HolidaysSJ): - pass +class SvalbardAndJanMayen(HolidaysSJ): + """Alias – long country name.""" + passApply similarly for
SJ
andSJM
.⛔ Skipped due to learnings
Learnt from: Wasif-Shahzad PR: vacanza/holidays#2593 File: holidays/countries/senegal.py:104-110 Timestamp: 2025-06-05T17:22:43.114Z Learning: In the holidays library codebase, country alias classes (like SN, SEN for Senegal) follow a consistent convention of having no docstrings and just containing "pass". This pattern is used across all countries - examples include BR/BRA for Brazil, CA/CAN for Canada, DE/DEU for Germany, etc. Static analysis tools may flag missing docstrings, but the established codebase convention is to keep these alias classes minimal.
Learnt from: KJhellico PR: vacanza/holidays#2609 File: holidays/countries/nauru.py:149-154 Timestamp: 2025-06-09T19:50:56.039Z Learning: In the holidays library project, never suggest adding docstrings for alias classes (like NR, NRU country code aliases that inherit from main country classes). The project deliberately omits docstrings for these simple alias classes.
tests/countries/test_svalbard_and_jan_mayen.py (3)
27-41
: Nice multi-language coverage.
The default-language block checks every 2022 public holiday – solid.
43-92
: 👍 Thorough l10n checks for en_US, uk, th.Great to see every locale validated, prevents future .po regressions.
21-23
: AlignsetUpClass
signature withunittest.TestCase
.Pylint warns (W0221) because the parent signature is
def setUpClass(cls)
.
Adding*args, **kwargs
silences the warning and keeps future refactors safe.- def setUpClass(cls): - super().setUpClass(HolidaysSJ) + def setUpClass(cls, *args, **kwargs): # type: ignore[override] + super().setUpClass(HolidaysSJ, *args, **kwargs)⛔ Skipped due to learnings
Learnt from: KJhellico PR: vacanza/holidays#2609 File: tests/countries/test_nauru.py:20-24 Timestamp: 2025-06-13T15:15:25.128Z Learning: In the vacanza/holidays test suite, overriding `setUpClass` is intentionally done with the single `cls` parameter (no *args/**kwargs), so signature-mismatch lint warnings are ignored project-wide.
snapshots/countries/NO_54.json (1)
1-1205
: Large static snapshot – no actionable issues.Verified keys look correct and alphabetically consistent; nothing to flag.
snapshots/countries/NO_34.json (1)
1-1205
: Ditto – data-only snapshot.No code to review here.
snapshots/countries/NO_46.json (2)
1-1205
: Approve JSON snapshot for subdivision NO_46.
The file follows the established snapshot pattern: date-sorted keys, no trailing commas, and covers the full range from 1950-01-01 to 2050-12-26.
1-1205
: Verify registry integration for region 46.
Please confirm thatNO_46.json
is correctly referenced by the subdivision code46
in your country registry and inholidays/countries/norway.py
.snapshots/countries/NO_15.json (2)
1-1205
: Approve JSON snapshot for subdivision NO_15.
Matches the national holiday set exactly, is valid JSON (no trailing commas), and spans 1950-01-01 through 2050-12-26.
1-1205
: Verify mapping for Svalbard (NO-15).
Ensure that subdivision code15
(Svalbard) is correctly wired to this snapshot in your newsvalbard_and_jan_mayen
module and the central registry.snapshots/countries/NO_21.json (2)
1-1206
: Missing Jan Mayen snapshot?
I see a snapshot for Svalbard (NO_21.json
) but none for Jan Mayen (NO_22.json
), even though the PR description covers both. Did you intend to include it?
1-1206
: Snapshot looks good
JSON is valid, chronologically sorted, and consistent with existing files.snapshots/countries/SJ_COMMON.json (2)
1-1206
: LGTM!
The SJ_COMMON.json file covers the full 1950–2050 range with correctly formatted dates and semicolon separators for multiple holidays. Ready to merge.
1-1206
: ```shell
#!/bin/bashCheck for a snapshot target in the Makefile to confirm how snapshots are generated
grep -R "snapshot" -n Makefile
</details> </blockquote></details> </details> <!-- This is an auto-generated comment by CodeRabbit for review status -->
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM 🇸🇯
Proposed change
Add Svalbard and Jan Mayen holidays (
en_US
,no
,th
,uk
l10n).Resolves #2477 .
Note
I originally assigned this task to myself to add the Norwegian Flag Days (which can be quite challenging for new contributors - see #2537), but due to circumstances I didn't have time to work on those yet - so I'm releasing the finished code as-is for now and adding them in later PRs instead.
Type of change
holidays
functionality in general)Checklist
make check
, all checks and tests are green