Update Canada holidays: add historical holidays #2507
Conversation
Summary by CodeRabbit
Summary by CodeRabbit
WalkthroughThis update refines the handling of Canadian Thanksgiving and Armistice Day, ensuring historical accuracy in both the holiday logic and test coverage. The Changes
Suggested reviewers
📜 Recent review detailsConfiguration used: .coderabbit.yaml 📒 Files selected for processing (1)
🧰 Additional context used🧠 Learnings (1)tests/countries/test_canada.py (1)🔇 Additional comments (5)
✨ Finishing Touches
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:
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 #2507 +/- ##
=========================================
Coverage 100.00% 100.00%
=========================================
Files 215 215
Lines 13600 13598 -2
Branches 1931 1924 -7
=========================================
- Hits 13600 13598 -2 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (16)
holidays/countries/canada.py(13 hunks)snapshots/countries/CA_AB.json(3 hunks)snapshots/countries/CA_BC.json(3 hunks)snapshots/countries/CA_COMMON.json(3 hunks)snapshots/countries/CA_MB.json(3 hunks)snapshots/countries/CA_NB.json(3 hunks)snapshots/countries/CA_NL.json(3 hunks)snapshots/countries/CA_NS.json(3 hunks)snapshots/countries/CA_NT.json(3 hunks)snapshots/countries/CA_NU.json(3 hunks)snapshots/countries/CA_ON.json(3 hunks)snapshots/countries/CA_PE.json(3 hunks)snapshots/countries/CA_QC.json(3 hunks)snapshots/countries/CA_SK.json(3 hunks)snapshots/countries/CA_YT.json(3 hunks)tests/countries/test_canada.py(33 hunks)
🧰 Additional context used
🧠 Learnings (1)
tests/countries/test_canada.py (1)
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.
🧬 Code Graph Analysis (1)
tests/countries/test_canada.py (2)
holidays/countries/canada.py (1)
Canada(30-506)tests/common.py (5)
assertHolidayName(195-199)assertNoNonObservedHoliday(248-250)assertNoHoliday(244-246)assertHoliday(150-152)assertNoHolidayName(273-275)
⏰ Context from checks skipped due to timeout of 300000ms (1)
- GitHub Check: Test build on windows-latest
🔇 Additional comments (42)
holidays/countries/canada.py (8)
44-47: Good addition of authoritative and archived sources.These new references properly document the historical context for Remembrance Day, Thanksgiving Day, and Victoria Day. Using web archive links ensures long-term availability.
156-174: Well-implemented historical correction for Thanksgiving and Armistice Day.The implementation accurately captures the historical relationship between Thanksgiving Day and Armistice Day. The method properly handles the special case of 1935 Thanksgiving, which occurred on October 24 due to a General Election, and correctly implements Armistice Day for 1921-1930. The detailed docstring provides excellent historical context.
The date change from October 25 to October 24 in 1935 is historically accurate according to the cited Canadian Encyclopedia reference.
175-188: Excellent refactoring of Victoria Day logic.This new dedicated method centralizes all Victoria Day logic that was previously scattered across multiple methods. The implementation correctly handles the historical rules:
- Before 1953: May 24 (with Sunday observance rules)
- 1953 onwards: First Monday before May 24
The detailed docstring provides clear historical context.
118-118: Good refactoring to use centralized Victoria Day method.This change replaces direct conditional implementation with the new centralized method.
194-194: Consistent implementation of Victoria Day across subdivisions.Changed all direct handling of Victoria Day to use the new centralized method. This improves code maintainability and ensures consistent handling across all Canadian subdivisions.
Also applies to: 230-230, 255-255, 288-288, 349-349, 370-370, 404-404, 462-462, 479-479
137-141: Improved formatting of Boxing Day handling.Added helpful comments and trailing commas that improve code readability.
144-148: Consistent formatting for Christmas Day handling.Added helpful comments and trailing commas for consistent style.
Also applies to: 150-154
168-169: Fixed 1935 Thanksgiving Day date.Corrected the 1935 Thanksgiving Day date from October 25 to October 24, which is historically accurate according to the Canadian Encyclopedia reference.
snapshots/countries/CA_AB.json (1)
6-6: Added missing Victoria Day entries for 1950-1952.These additions correctly implement the Victoria Day holiday for years 1950-1952 based on the historical rule that Victoria Day was observed on May 24 prior to 1953. This change brings the snapshot in line with the corrected
_add_victoria_day()implementation.Also applies to: 18-18, 30-30
snapshots/countries/CA_YT.json (1)
5-5: Added missing Victoria Day entries for 1950-1952.These additions correctly implement the Victoria Day holiday for years 1950-1952 based on the historical rule that Victoria Day was observed on May 24 prior to 1953. This change ensures consistency with the updated
_add_victoria_day()implementation.Also applies to: 17-17, 29-29
snapshots/countries/CA_BC.json (1)
5-5: Added missing Victoria Day entries for 1950-1952.These additions correctly implement the Victoria Day holiday for years 1950-1952 based on the historical rule that Victoria Day was observed on May 24 prior to 1953. This ensures all provinces and territories have consistent historical holiday data.
Also applies to: 16-16, 27-27
snapshots/countries/CA_ON.json (1)
5-5: Approve snapshot additions for Victoria Day
The entries for"1950-05-24","1951-05-24", and"1952-05-24"correctly reflect Victoria Day prior to the 1953 rule change. They are inserted in proper chronological order and align with the new private_add_victoria_day()logic.Also applies to: 17-17, 29-29
snapshots/countries/CA_SK.json (1)
5-5: Approve snapshot additions for Victoria Day
The added"1950-05-24","1951-05-24", and"1952-05-24"entries match the centralized Victoria Day rules and maintain correct ordering in the Saskatchewan snapshot.Also applies to: 17-17, 29-29
snapshots/countries/CA_COMMON.json (1)
5-5: Approve snapshot additions for Victoria Day
Adding"1950-05-24","1951-05-24", and"1952-05-24"in the common snapshot fills in the pre-1953 Victoria Day dates in line with the refactored logic. Order and consistency look good.Also applies to: 16-16, 27-27
snapshots/countries/CA_PE.json (1)
5-5: Approve snapshot additions for Victoria Day
The Prince Edward Island snapshot now includes"1950-05-24","1951-05-24", and"1952-05-24"for Victoria Day, correctly mirroring the new historical rule implementation.Also applies to: 16-16, 27-27
snapshots/countries/CA_QC.json (3)
6-6: Approve missing Victoria Day for 1950.
Entry"1950-05-24": "Victoria Day"correctly reflects the pre-1953 rule.
19-19: Approve missing Victoria Day for 1951.
Entry"1951-05-24": "Victoria Day"is accurate and in line with the new_add_victoria_day()logic.
33-33: Approve missing Victoria Day for 1952.
Entry"1952-05-24": "Victoria Day"matches historical observance prior to the shift in 1953.snapshots/countries/CA_NT.json (3)
5-5: Approve missing Victoria Day for 1950 (NT).
Snapshot now includes"1950-05-24": "Victoria Day", consistent with other provinces.
17-17: Approve missing Victoria Day for 1951 (NT).
Entry"1951-05-24": "Victoria Day"aligns with the shared pre-1953 rule.
29-29: Approve missing Victoria Day for 1952 (NT).
Entry"1952-05-24": "Victoria Day"correctly captures the historical date.snapshots/countries/CA_NL.json (3)
6-6: Approve missing Victoria Day for 1950 (NL).
Snapshot now correctly lists"1950-05-24": "Victoria Day".
19-19: Approve missing Victoria Day for 1951 (NL).
Entry"1951-05-24": "Victoria Day"follows the unified logic.
32-32: Approve missing Victoria Day for 1952 (NL).
Adding"1952-05-24": "Victoria Day"is consistent with the new rule.snapshots/countries/CA_NU.json (3)
5-5: Approve missing Victoria Day for 1950 (NU).
The snapshot entry"1950-05-24": "Victoria Day"is correct.
17-17: Approve missing Victoria Day for 1951 (NU).
Entry"1951-05-24": "Victoria Day"matches the centralized logic.
29-29: Approve missing Victoria Day for 1952 (NU).
Adding"1952-05-24": "Victoria Day"aligns with the historical observance.snapshots/countries/CA_NB.json (3)
5-5: Good addition of missing Victoria Day for 1950.This accurately adds Victoria Day on May 24, 1950, which was previously missing. This follows the historical fixed date rule before Victoria Day changed to a floating Monday in 1953.
16-16: Properly adds Victoria Day for 1951.Adding Victoria Day on May 24, 1951 maintains historical accuracy for the period when Victoria Day was celebrated on a fixed date rather than the floating date system that started in 1953.
27-27: Correctly adds Victoria Day for 1952.Including Victoria Day on May 24, 1952 completes the set of missing Victoria Day holidays for the years immediately preceding the 1953 change to a floating Monday celebration.
snapshots/countries/CA_MB.json (3)
5-5: Good addition of Victoria Day for 1950.This properly adds the Victoria Day holiday for Manitoba on May 24, 1950, maintaining consistency with other Canadian provinces and historical accuracy.
17-17: Victoria Day for 1951 correctly added.Adding Victoria Day for Manitoba on May 24, 1951 is historically accurate. This ensures Manitoba's holiday data is consistent with the fixed date observation of Victoria Day during this period.
29-29: Victoria Day for 1952 properly included.The addition of Victoria Day on May 24, 1952 for Manitoba completes the set of missing holiday data for the years before Victoria Day switched to a floating date celebration.
snapshots/countries/CA_NS.json (3)
5-5: Victoria Day for 1950 properly added.The addition of Victoria Day on May 24, 1950 for Nova Scotia aligns with the historical observance and ensures consistency with other provinces.
16-16: Victoria Day for 1951 correctly included.Adding Victoria Day for May 24, 1951 in Nova Scotia's holiday data helps maintain historical accuracy for the fixed-date observance period.
27-27: Good addition of Victoria Day for 1952.Including Victoria Day on May 24, 1952 for Nova Scotia completes the historical data for the years before the holiday switched to a floating Monday celebration in 1953.
tests/countries/test_canada.py (6)
38-56: Good variable name refactoring for clarity.The change from
prov/prov_holstosubdiv/subdiv_holidaysprovides more accurate terminology and better aligns with theCanada.subdivisionsproperty. This improves code readability and maintainability.
223-264: Improved Victoria Day test coverage.Adding explicit tests for observed Victoria Day instances and improving the structure of assertions strengthens the test suite. The addition of
dts_obswith specific observed dates for Victoria Day is particularly valuable for ensuring the holiday behaves correctly when it falls on a Sunday.
309-369: Great historical accuracy improvement for Thanksgiving/Armistice Day.This significant enhancement adds proper testing for two distinct holiday periods:
- "Armistice Day" from 1921-1930
- "Thanksgiving Day" from 1931 onward
The test also corrects a historical date for 1935 Thanksgiving (changing from October 25th to October 24th). This improves historical accuracy and provides better test coverage for this important Canadian holiday.
390-404: Improved Remembrance Day tests with better subdivision handling.The updated test structure for Remembrance Day better handles subdivision-specific behavior, separating provinces that have observed date shifts from those that don't. This makes the tests more readable and maintainable.
824-830: Updated Queen's funeral test with new variable names.Correctly modified the Queen's funeral test to use the new
subdiv/subdiv_holidaysvariable names while maintaining the original test logic.
971-1005: Armistice Day properly reflected in localization tests.The localization tests have been properly updated to include Armistice Day, ensuring that translations for this holiday are available in multiple languages. This is important for historical completeness.
Co-authored-by: ~Jhellico <KJhellico@users.noreply.github.com> Signed-off-by: Panpakorn Siripanich <19505219+PPsyrius@users.noreply.github.com>
|
Proposed change
While cleaning up outdated
tiny.ccsource link aliases as part of #2504, I've noticed that our source doesn't match other online sources. Additionally, "Armistice Day" was the combined precursor of both Thanksgiving and Remembrance Day for 1921-1930 so I've added them in too.Type of change
holidaysfunctionality in general)Checklist
make check, all checks and tests are green