Add USGS and USNY financial market holiday calendars #3008
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Implement support for US Government Securities (USGS) and US New York (USNY) financial market holiday calendars following SIFMA recommendations. - Add USGovernmentSecurities class with full bond market holidays including Good Friday, Columbus Day, and Veterans Day - Add USNewYork class with standard New York financial market holidays - Include proper observed date handling (SAT_TO_PREV_FRI + SUN_TO_NEXT_MON) - Comprehensive test coverage with 44 passing tests across both calendars - Support for holidays from 1950 onwards with proper historical transitions - Include Juneteenth support starting 2021 and MLK Day starting 1998 Fixes vacanza#852
Summary by CodeRabbit
WalkthroughThis pull request adds support for two new financial holiday calendars: US Government Securities (USGS) and US New York (USNY), following SIFMA recommendations. It introduces two new modules with calendar implementations, public API exports, comprehensive test coverage, registry entries, and documentation updates. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes
Suggested reviewers
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro 📒 Files selected for processing (2)
🧰 Additional context used🧠 Learnings (44)📓 Common learnings📚 Learning: 2025-08-11T13:48:45.953ZApplied to files:
📚 Learning: 2025-04-03T16:58:27.175ZApplied to files:
📚 Learning: 2025-06-19T05:54:49.792ZApplied to files:
📚 Learning: 2025-07-02T18:17:53.342ZApplied to files:
📚 Learning: 2025-06-16T12:28:31.641ZApplied to files:
📚 Learning: 2025-06-10T12:43:10.577ZApplied to files:
📚 Learning: 2025-08-26T20:10:05.288ZApplied to files:
📚 Learning: 2025-10-28T17:26:45.079ZApplied to files:
📚 Learning: 2025-08-08T14:37:03.045ZApplied to files:
📚 Learning: 2025-06-16T11:42:28.293ZApplied to files:
📚 Learning: 2025-08-11T10:14:28.517ZApplied to files:
📚 Learning: 2025-08-30T12:52:58.539ZApplied to files:
📚 Learning: 2025-07-09T21:16:35.145ZApplied to files:
📚 Learning: 2025-04-23T14:55:35.504ZApplied to files:
📚 Learning: 2025-08-22T19:06:04.303ZApplied to files:
📚 Learning: 2025-10-28T17:02:23.968ZApplied to files:
📚 Learning: 2025-04-23T09:59:19.886ZApplied to files:
📚 Learning: 2025-10-21T09:59:18.714ZApplied to files:
📚 Learning: 2025-06-06T14:40:31.932ZApplied to files:
📚 Learning: 2025-04-25T20:27:59.086ZApplied to files:
📚 Learning: 2025-06-14T11:04:31.180ZApplied to files:
📚 Learning: 2025-06-14T20:43:15.370ZApplied to files:
📚 Learning: 2025-07-24T15:21:31.632ZApplied to files:
📚 Learning: 2025-04-08T14:50:15.325ZApplied to files:
📚 Learning: 2025-06-14T21:12:07.224ZApplied to files:
📚 Learning: 2025-06-18T10:10:46.158ZApplied to files:
📚 Learning: 2025-05-09T18:36:09.607ZApplied to files:
📚 Learning: 2025-05-12T15:31:58.079ZApplied to files:
📚 Learning: 2025-06-16T15:48:48.680ZApplied to files:
📚 Learning: 2025-04-05T06:49:06.217ZApplied to files:
📚 Learning: 2025-05-06T21:07:11.577ZApplied to files:
📚 Learning: 2025-06-15T11:52:39.572ZApplied to files:
📚 Learning: 2025-03-04T11:32:45.095ZApplied to files:
📚 Learning: 2025-03-04T11:32:45.095ZApplied to files:
📚 Learning: 2025-09-20T12:24:28.864ZApplied to files:
📚 Learning: 2025-08-25T10:51:08.068ZApplied to files:
📚 Learning: 2025-08-03T13:48:11.910ZApplied to files:
📚 Learning: 2025-03-05T02:35:03.298ZApplied to files:
📚 Learning: 2025-03-04T11:41:56.389ZApplied to files:
📚 Learning: 2025-09-14T07:26:25.431ZApplied to files:
📚 Learning: 2025-09-07T19:28:19.153ZApplied to files:
📚 Learning: 2025-09-16T04:16:10.671ZApplied to files:
📚 Learning: 2025-03-19T16:54:58.657ZApplied to files:
🧬 Code graph analysis (2)holidays/financial/us_new_york.py (5)
holidays/financial/us_government_securities.py (2)
🪛 Ruff (0.14.2)holidays/financial/us_new_york.py62-62: Missing return type annotation for special method Add return type annotation: (ANN204) 62-62: Missing type annotation for (ANN002) 62-62: Missing type annotation for (ANN003) 68-68: Missing return type annotation for private function Add return type annotation: (ANN202) 122-122: Missing return type annotation for private function Add return type annotation: (ANN202) holidays/financial/us_government_securities.py43-43: Missing return type annotation for private function Add return type annotation: (ANN202) 52-52: Missing return type annotation for private function Add return type annotation: (ANN202) 🔇 Additional comments (2)
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
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (6)
CONTRIBUTORS(1 hunks)holidays/financial/__init__.py(1 hunks)holidays/financial/us_government_securities.py(1 hunks)holidays/financial/us_new_york.py(1 hunks)tests/financial/test_us_government_securities.py(1 hunks)tests/financial/test_us_new_york.py(1 hunks)
🧰 Additional context used
🧠 Learnings (5)
📚 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/financial/test_us_new_york.pytests/financial/test_us_government_securities.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/financial/test_us_new_york.pytests/financial/test_us_government_securities.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/financial/test_us_new_york.pytests/financial/test_us_government_securities.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/financial/test_us_government_securities.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/financial/test_us_government_securities.py
🧬 Code graph analysis (5)
holidays/financial/us_new_york.py (4)
holidays/groups/christian.py (1)
_add_christmas_day(224-232)holidays/groups/international.py (1)
_add_new_years_day(142-150)holidays/observed_holiday_base.py (2)
ObservedHolidayBase(102-256)_move_holiday(199-211)holidays/financial/us_government_securities.py (1)
_populate_public_holidays(44-86)
holidays/financial/__init__.py (2)
holidays/financial/us_government_securities.py (3)
USGovernmentSecurities(17-86)USGS(89-90)USBondMarket(93-94)holidays/financial/us_new_york.py (3)
USNewYork(17-75)USNY(78-79)USNewYorkFinancial(82-83)
holidays/financial/us_government_securities.py (4)
holidays/groups/christian.py (2)
_add_good_friday(344-353)_add_christmas_day(224-232)holidays/groups/international.py (2)
_add_new_years_day(142-150)_add_remembrance_day(182-190)holidays/observed_holiday_base.py (1)
_move_holiday(199-211)holidays/financial/us_new_york.py (1)
_populate_public_holidays(45-75)
tests/financial/test_us_new_york.py (3)
tests/common.py (7)
TestCase(31-370)CommonFinancialTests(434-439)assertAliases(124-133)assertNoHolidays(324-326)assertHoliday(153-155)assertNoHoliday(276-278)assertHolidays(231-233)holidays/calendars/gregorian.py (1)
_timedelta(37-42)holidays/financial/us_new_york.py (3)
USNewYork(17-75)USNY(78-79)USNewYorkFinancial(82-83)
tests/financial/test_us_government_securities.py (3)
tests/common.py (7)
TestCase(31-370)CommonFinancialTests(434-439)assertAliases(124-133)assertNoHolidays(324-326)assertHoliday(153-155)assertNoHoliday(276-278)assertHolidays(231-233)holidays/calendars/gregorian.py (1)
_timedelta(37-42)holidays/financial/us_government_securities.py (3)
USGovernmentSecurities(17-86)USGS(89-90)USBondMarket(93-94)
🪛 Ruff (0.14.0)
holidays/financial/us_new_york.py
39-39: Missing return type annotation for special method __init__
Add return type annotation: None
(ANN204)
39-39: Missing type annotation for *args
(ANN002)
39-39: Missing type annotation for **kwargs
(ANN003)
45-45: Missing return type annotation for private function _populate_public_holidays
Add return type annotation: None
(ANN202)
holidays/financial/us_government_securities.py
38-38: Missing return type annotation for special method __init__
Add return type annotation: None
(ANN204)
38-38: Missing type annotation for *args
(ANN002)
38-38: Missing type annotation for **kwargs
(ANN003)
44-44: Missing return type annotation for private function _populate_public_holidays
Add return type annotation: None
(ANN202)
tests/financial/test_us_new_york.py
23-23: Missing return type annotation for classmethod setUpClass
Add return type annotation: None
(ANN206)
tests/financial/test_us_government_securities.py
27-27: Missing return type annotation for classmethod setUpClass
Add return type annotation: None
(ANN206)
🔇 Additional comments (17)
CONTRIBUTORS (1)
138-138: LGTM!Contributor entry is correctly placed alphabetically and follows the established format.
holidays/financial/us_new_york.py (3)
39-43: LGTM!The initialization follows the established pattern for financial holiday calendars in the codebase, correctly setting up observed rules and calling parent initializers.
45-75: LGTM!Holiday implementation correctly handles:
- Observed date adjustments for fixed-date holidays using
_move_holiday- Year-based guards for MLK Day (1998+) and Juneteenth (2021+)
- Proper distinction from USGS calendar (excluding Good Friday, Columbus Day, Veterans Day)
The logic matches test expectations and SIFMA conventions for New York equity markets.
78-83: LGTM!Alias classes provide alternative names for the calendar, following the established pattern in the codebase.
tests/financial/test_us_government_securities.py (6)
25-31: LGTM!Test setup and alias validation follow the established testing pattern for financial calendars.
33-34: LGTM!Correctly verifies no holidays before the calendar's start year.
36-48: LGTM!Test correctly verifies New Year's Day observance rules, including when January 1 falls on Sunday (observed Monday) and weekdays.
49-62: LGTM!Correctly verifies MLK Day observance starting 1998 and validates proper 3rd Monday of January calculation across multiple years.
64-183: LGTM!All individual holiday tests provide comprehensive coverage:
- Verify correct calculation of movable holidays (Washington's Birthday, Good Friday, Memorial Day, etc.)
- Test observed date adjustments for fixed holidays (Independence Day, Veterans Day, Christmas)
- Include year guards for historically introduced holidays (Juneteenth 2021+)
- Validate adjacency to ensure no false positives
Test dates span historical range (1950-2023) and logic correctly handles both actual and observed date scenarios.
184-230: LGTM!Year-specific tests provide comprehensive validation of all holidays for key years (2023-2025), including proper observed labels. This dual-testing approach complements individual holiday tests and aligns with project conventions.
Based on learnings
holidays/financial/__init__.py (1)
24-29: LGTM!New calendar classes and aliases are correctly exported, following the established pattern for financial calendar modules. Alphabetical ordering is maintained.
tests/financial/test_us_new_york.py (2)
21-144: LGTM!Test structure and coverage mirror the USGS tests with appropriate adjustments for USNY:
- Correct setup and alias validation
- Comprehensive individual holiday tests with adjacency checks
- Proper year guards for MLK Day (1998+) and Juneteenth (2021+)
- Accurate observed date handling
The tests correctly reflect the USNY calendar's exclusion of Good Friday, Columbus Day, and Veterans Day.
145-194: Excellent test coverage!The negative tests explicitly verify that Good Friday, Columbus Day, and Veterans Day are not observed in USNY, preventing regressions and clearly documenting the differences from USGS.
Year-specific tests correctly show 9 holidays (vs 12 for USGS), confirming the proper exclusions while maintaining comprehensive validation.
holidays/financial/us_government_securities.py (4)
17-36: LGTM!Class structure, documentation, and configuration match the established pattern. The docstring clearly explains SIFMA recommendations and provides proper references.
38-42: LGTM!Initialization correctly sets up observed rules and parent classes, matching the pattern established in USNY.
44-86: LGTM!Holiday implementation correctly includes all USGS bond market holidays:
- Matches USNY for common holidays (consistent logic)
- Properly adds Good Friday, Columbus Day, and Veterans Day (bond market specifics)
- Uses correct year guards and observed date handling
- Inline comments explain non-federal holidays
The implementation accurately reflects SIFMA bond market conventions.
89-94: LGTM!Alias classes follow the established pattern, providing alternative names for the calendar.
Sort CONTRIBUTORS file to pass pre-commit file-contents-sorter check. Moved names with special characters (Joël, Søren) to correct positions.
- Fix line length in test_us_government_securities.py (split long import) - Fix CONTRIBUTORS sorting for special characters (Joël, Søren)
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (2)
CONTRIBUTORS(1 hunks)tests/financial/test_us_government_securities.py(1 hunks)
🧰 Additional context used
🧠 Learnings (5)
📚 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/financial/test_us_government_securities.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/financial/test_us_government_securities.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/financial/test_us_government_securities.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/financial/test_us_government_securities.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/financial/test_us_government_securities.py
🧬 Code graph analysis (1)
tests/financial/test_us_government_securities.py (3)
tests/common.py (7)
TestCase(31-370)CommonFinancialTests(434-439)assertAliases(124-133)assertNoHolidays(324-326)assertHoliday(153-155)assertNoHoliday(276-278)assertHolidays(231-233)holidays/calendars/gregorian.py (1)
_timedelta(37-42)holidays/financial/us_government_securities.py (3)
USGovernmentSecurities(17-86)USGS(89-90)USBondMarket(93-94)
🪛 Ruff (0.14.0)
tests/financial/test_us_government_securities.py
39-39: Missing return type annotation for classmethod setUpClass
Add return type annotation: None
(ANN206)
🔇 Additional comments (7)
CONTRIBUTORS (1)
138-138: LGTM!Welcome to the contributors list!
tests/financial/test_us_government_securities.py (6)
13-34: LGTM!Imports are clean and appropriate for financial holiday testing.
42-46: LGTM!Alias testing and pre-start-year validation are correct.
48-110: LGTM!Holiday tests are comprehensive and properly structured:
- New Year's Day with observed date handling
- MLK Day with correct 1998 start year
- Washington's Birthday
- Good Friday
- Memorial Day
Each test validates multiple years and checks adjacent dates to ensure isolation.
112-124: LGTM!Juneteenth testing correctly validates the 2021 start year and includes proper observed date handling.
125-194: LGTM!Holiday tests for Independence Day through Christmas are thorough:
- Proper observed date handling for fixed holidays (July 4, Nov 11, Dec 25)
- Correct testing of floating holidays (Labor Day, Columbus Day, Veterans Day, Thanksgiving)
- Adjacent date checks ensure holidays are isolated
The conditional logic in lines 135, 170, and 194 correctly handles observed dates that fall on different days.
196-242: LGTM!Comprehensive year tests validate all holidays for 2023-2025 with correct dates and observed labels. These complement the individual holiday tests per project conventions.
Add us_government_securities and us_new_york to FINANCIAL registry dict to enable market discovery via list_supported_financial(). Fixes test_list_supported_financial assertion error (expected 7 markets, was getting 5).
- Added US Government Securities (USGS) market entry - Added US New York (USNY) market entry - Both entries include market codes and descriptions Fixes test_supported_markets_table test failure. 🤖 Generated with [Claude Code](https://claude.com/claude-code)
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## dev #3008 +/- ##
=========================================
Coverage 100.00% 100.00%
=========================================
Files 306 308 +2
Lines 18076 18138 +62
Branches 2334 2340 +6
=========================================
+ Hits 18076 18138 +62 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
KJhellico
reviewed
Oct 18, 2025
Address maintainer feedback (@KJhellico): 1. Created SIFMAHolidays base class with common SIFMA holiday logic 2. USGS inherits from SIFMAHolidays (includes all SIFMA holidays) 3. USNY inherits from SIFMAHolidays but excludes Good Friday, Columbus Day, and Veterans Day (documented differences from USGS) Key differences between USGS and USNY: - USGS: Full calendar following SIFMA recommendations - USNY: Subset of USGS, excludes Good Friday, Columbus Day, Veterans Day This refactoring reduces code duplication and makes the relationship between these calendars clearer. Note: HALF_DAY early close holidays can be implemented in a future update. 🤖 Generated with [Claude Code](https://claude.com/claude-code)
There was a problem hiding this comment.
Actionable comments posted: 2
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (1)
holidays/financial/sifma.py(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
holidays/financial/sifma.py (4)
holidays/groups/christian.py (2)
_add_good_friday(344-353)_add_christmas_day(224-232)holidays/groups/international.py (2)
_add_new_years_day(142-150)_add_remembrance_day(182-190)holidays/observed_holiday_base.py (1)
_move_holiday(199-211)holidays/financial/us_new_york.py (1)
_populate_public_holidays(47-56)
🪛 Ruff (0.14.0)
holidays/financial/sifma.py
40-40: Missing return type annotation for special method __init__
Add return type annotation: None
(ANN204)
40-40: Missing type annotation for *args
(ANN002)
40-40: Missing type annotation for **kwargs
(ANN003)
46-46: Missing return type annotation for private function _populate_public_holidays
Add return type annotation: None
(ANN202)
🔇 Additional comments (3)
holidays/financial/sifma.py (3)
66-70: Observed handling for fixed-date holidays looks good; minor naming consistency check.Juneteenth, Independence Day, Veterans Day, and Christmas use _move_holiday correctly. Verify that naming matches existing US modules (e.g., “Washington’s Birthday” vs “Presidents Day”) for consistency in downstream tooling.
Also applies to: 71-73, 81-84, 88-89
43-44: ObservedRule composition verified—no action needed.The
__add__method is defined forObservedRule(holidays/observed_holiday_base.py:23), so the expressionSAT_TO_PREV_FRI + SUN_TO_NEXT_MONis properly supported.
47-49: Add test coverage for New Year's Day observed behavior with single-year calendars.The codebase includes no SIFMA tests. When Jan 1 falls on Saturday (e.g., 2022), the observed date (2021-12-31) is correctly added to calendars, but this cross-year behavior is never explicitly tested. Add tests asserting:
SIFMA(years=2022)includes 2021-12-31 as "New Year's Day (observed)"SIFMA(years=2017)includes 2017-01-02 as "New Year's Day (observed)" (Jan 1, 2017 is Sunday)
Renamed sifma.py to _sifma.py to indicate it's a base class, not a market implementation. Updated test_list_supported_financial to exclude private modules (starting with underscore) from the market count. This resolves the AssertionError where the test counted 8 files but only 7 registered markets, as _sifma.py should not be counted as a market. 🤖 Generated with [Claude Code](https://claude.com/claude-code)
Add support for early close days (markets close at 2:00 PM ET) as recommended by SIFMA for bond markets. Early close days include: - Day before New Year's Day (if weekday) - Day before Good Friday (Thursday before Easter) - Day before Memorial Day (Friday before last Monday of May) - Day before Independence Day (if weekday) - Day after Thanksgiving (Black Friday) - Day before Christmas Day (if weekday) Both USGS and USNY now support HALF_DAY category. USNY correctly excludes the early close day before Good Friday since Good Friday itself is not observed as a full closure in the New York market. References: - https://www.sifma.org/resources/general/holiday-schedule/ - https://www.nyse.com/markets/hours-calendars 🤖 Generated with [Claude Code](https://claude.com/claude-code)
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
holidays/financial/_sifma.py (1)
43-47: Add missing type annotations (ruff ANN002/ANN003/ANN204/ANN202).Annotate init, populators, and the inner helper; import Any/date.
@@ -from holidays.calendars.gregorian import _timedelta +from datetime import date +from typing import Any +from holidays.calendars.gregorian import _timedelta @@ - def __init__(self, *args, **kwargs): + def __init__(self, *args: Any, **kwargs: Any) -> None: @@ - def _populate_public_holidays(self): + def _populate_public_holidays(self) -> None: @@ - def _populate_half_day_holidays(self): + def _populate_half_day_holidays(self) -> None: @@ - def add_early_close(holiday_name, delta_days): + def add_early_close(holiday_name: str, delta_days: int) -> None:Also applies to: 49-49, 94-94, 100-100
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (1)
holidays/financial/_sifma.py(1 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 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:
holidays/financial/_sifma.py
🧬 Code graph analysis (1)
holidays/financial/_sifma.py (5)
holidays/calendars/gregorian.py (1)
_timedelta(37-42)holidays/groups/christian.py (2)
_add_good_friday(344-353)_add_christmas_day(224-232)holidays/groups/international.py (2)
_add_new_years_day(142-150)_add_remembrance_day(182-190)holidays/observed_holiday_base.py (2)
ObservedHolidayBase(102-256)_move_holiday(199-211)holidays/holiday_base.py (3)
get_named(998-1054)_is_weekday(868-873)_add_holiday(797-809)
🪛 Ruff (0.14.0)
holidays/financial/_sifma.py
43-43: Missing return type annotation for special method __init__
Add return type annotation: None
(ANN204)
43-43: Missing type annotation for *args
(ANN002)
43-43: Missing type annotation for **kwargs
(ANN003)
49-49: Missing return type annotation for private function _populate_public_holidays
Add return type annotation: None
(ANN202)
94-94: Missing return type annotation for private function _populate_half_day_holidays
Add return type annotation: None
(ANN202)
100-100: Missing return type annotation for private function add_early_close
Add return type annotation: None
(ANN202)
🔇 Additional comments (2)
holidays/financial/_sifma.py (2)
49-93: Public holiday set looks solid.MLK since 1998 and Juneteenth since 2021 align with market practice; observed rules applied consistently. LGTM.
Add a quick assertion in tests that observed labels appear when holidays fall on weekends (e.g., 2026‑07‑04, 2032‑12‑25).
94-124: Add tests for HALF_DAY scenarios to lock behavior.Cover:
- Memorial Day Friday early close (e.g., 2024‑05‑24).
- Independence Day on Sunday (2021) ⇒ Friday 2021‑07‑02 early close.
- Christmas on Monday (2017/2023) ⇒ Friday early close.
- New Year’s Eve derived from next year’s New Year’s (e.g., 2020‑12‑31, 2021‑12‑31).
USNY should only exclude Good Friday (keeping Columbus Day and Veterans Day). This matches the OpenGamma Strata implementation which defines USNY as general "New York business days" used in financial contracts. Key differences: - USNY excludes Good Friday (unlike NYSE and USGS which include it) - USNY includes Columbus Day and Veterans Day (unlike NYSE which doesn't) - USNY doesn't include special one-off closures (unlike NYSE which does) This properly distinguishes USNY from NYSE: - USNY: General NY business days for ISDA derivatives and OTC instruments - NYSE: Stock exchange trading calendar with special closures Updated tests to verify Columbus Day and Veterans Day are included. References: - OpenGamma Strata source: https://github.com/OpenGamma/Strata/blob/main/modules/basics/src/main/java/com/opengamma/strata/basics/date/GlobalHolidayCalendars.java 🤖 Generated with [Claude Code](https://claude.com/claude-code)
There was a problem hiding this comment.
Actionable comments posted: 2
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (2)
holidays/financial/us_new_york.py(1 hunks)tests/financial/test_us_new_york.py(1 hunks)
🧰 Additional context used
🧠 Learnings (5)
📚 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/financial/test_us_new_york.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/financial/test_us_new_york.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/financial/test_us_new_york.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/financial/test_us_new_york.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/financial/test_us_new_york.py
🧬 Code graph analysis (2)
holidays/financial/us_new_york.py (2)
holidays/financial/_sifma.py (2)
SIFMAHolidays(19-124)_populate_public_holidays(49-92)holidays/holiday_base.py (1)
pop(1173-1202)
tests/financial/test_us_new_york.py (3)
tests/common.py (6)
TestCase(31-370)assertAliases(124-133)assertNoHolidays(324-326)assertHoliday(153-155)assertNoHoliday(276-278)assertHolidays(231-233)holidays/calendars/gregorian.py (1)
_timedelta(37-42)holidays/financial/us_new_york.py (3)
USNewYork(16-62)USNY(65-66)USNewYorkFinancial(69-70)
🪛 Ruff (0.14.1)
holidays/financial/us_new_york.py
48-48: Missing return type annotation for private function _populate_public_holidays
Add return type annotation: None
(ANN202)
58-58: Missing return type annotation for private function _remove_holiday_by_name
Add return type annotation: None
(ANN202)
tests/financial/test_us_new_york.py
23-23: Missing return type annotation for classmethod setUpClass
Add return type annotation: None
(ANN206)
🔇 Additional comments (5)
holidays/financial/us_new_york.py (2)
16-46: Solid documentation and market identifier.The docstring clearly explains USNY's differences from USGS and NYSE, with appropriate references to SIFMA and OpenGamma Strata. The market identifier is correctly set.
65-70: Clean alias pattern.The alias classes follow project conventions and match the pattern used in other financial calendars.
tests/financial/test_us_new_york.py (3)
21-30: Test setup and basic validation look good.The class setup follows project patterns, alias verification is correct, and the no-holidays test properly validates behavior before the start year (1950).
32-173: Comprehensive individual holiday coverage.Each holiday is tested across relevant years with proper adjacency checks. Historical transitions (MLK Day from 1998, Juneteenth from 2021) are correctly validated. The tests for Good Friday (not observed), Columbus Day, and Veterans Day (both observed) align with the USNY specification.
175-218: Year-specific tests provide solid comprehensive validation.The 2023-2025 tests verify complete holiday lists with correct names and dates, following the project's dual testing approach (individual + year-specific).
KJhellico
reviewed
Oct 20, 2025
- Add Uniform Monday Holiday Act comments for Washington's Birthday, Memorial Day, Columbus Day, and Veterans Day - Rewrite _populate_half_day_holidays() to calculate dates independently instead of relying on PUBLIC holiday lookups, addressing maintainer requirement that categories must have independent implementations - Add comprehensive HALF_DAY tests for USGS and USNY to achieve 100% coverage - Add _populate_half_day_holidays() override in USNY to remove Good Friday early close since USNY does not observe Good Friday 🤖 Generated with [Claude Code](https://claude.com/claude-code)
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (5)
holidays/financial/_sifma.py (3)
98-156: HALF_DAY logic: NYE unreachable; Memorial offset wrong; anchor to observed; add weekend walk.
- New Year's: anchored to same-year Jan 1, yields prior-year Dec 31 and is dropped.
- Memorial Day: using -1 hits Sunday; should be Friday (-3) and normalize weekends.
- Independence/Christmas: base on observed date, not the fixed date.
- Keep HALF_DAY independent of PUBLIC; compute dates directly.
Option A (robust, may require test updates if NYE/Memorial are intended):
@@ - def _populate_half_day_holidays(self): + def _populate_half_day_holidays(self) -> None: @@ - from holidays.calendars.gregorian import ( - JAN, - JUL, - MAY, - MON, - NOV, - THU, - _get_nth_weekday_of_month, - date, - ) + from holidays.calendars.gregorian import ( + JAN, + JUL, + MAY, + MON, + NOV, + THU, + _get_nth_weekday_of_month, + date, + ) @@ - # Day before New Year's Day (if it's a weekday). - new_years = date(self._year, JAN, 1) - if self._is_weekday(new_years): - dt = _timedelta(new_years, -1) - if self._is_weekday(dt): - self._add_holiday("Markets close at 2:00 PM ET (New Year's Day)", dt) + def _walk_weekday(dt: date, step: int) -> date: + while not self._is_weekday(dt): + dt = _timedelta(dt, step) + return dt + + # Day before New Year's Day (anchor to next year's observed New Year's Day). + ny_observed = self._get_observed_date(date(self._year + 1, JAN, 1), self._observed_rule) + if ny_observed: + dt = _walk_weekday(_timedelta(ny_observed, -1), -1) + if dt.year == self._year: + self._add_holiday("Markets close at 2:00 PM ET (New Year's Day)", dt) @@ - # Day before Memorial Day (Friday before last Monday of May). - # Memorial Day is the last Monday of May. - memorial_day = _get_nth_weekday_of_month(-1, MON, MAY, self._year) - dt = _timedelta(memorial_day, -1) - if self._is_weekday(dt): - self._add_holiday("Markets close at 2:00 PM ET (Memorial Day)", dt) + # Friday before Memorial Day (last Monday of May). + memorial_mon = _get_nth_weekday_of_month(-1, MON, MAY, self._year) + dt = _walk_weekday(_timedelta(memorial_mon, -3), -1) + self._add_holiday("Markets close at 2:00 PM ET (Memorial Day)", dt) @@ - # Day before Independence Day (if it's a weekday). - independence_day = date(self._year, JUL, 4) - if self._is_weekday(independence_day): - dt = _timedelta(independence_day, -1) - if self._is_weekday(dt): - self._add_holiday("Markets close at 2:00 PM ET (Independence Day)", dt) + # Day before Independence Day (anchor to observed). + jul4_observed = self._get_observed_date(date(self._year, JUL, 4), self._observed_rule) + if jul4_observed: + dt = _walk_weekday(_timedelta(jul4_observed, -1), -1) + if dt.year == self._year: + self._add_holiday("Markets close at 2:00 PM ET (Independence Day)", dt) @@ - # Day before Christmas Day (if it's a weekday). - christmas = self._christmas_day - if self._is_weekday(christmas): - dt = _timedelta(christmas, -1) - if self._is_weekday(dt): - self._add_holiday("Markets close at 2:00 PM ET (Christmas Day)", dt) + # Day before Christmas Day (anchor to observed). + xmas_observed = self._get_observed_date(self._christmas_day, self._observed_rule) + if xmas_observed: + dt = _walk_weekday(_timedelta(xmas_observed, -1), -1) + if dt.year == self._year: + self._add_holiday("Markets close at 2:00 PM ET (Christmas Day)", dt)Option B (match current tests; remove unreachable NYE/Memorial blocks to avoid confusion):
@@ - # Day before New Year's Day (if it's a weekday). - new_years = date(self._year, JAN, 1) - if self._is_weekday(new_years): - dt = _timedelta(new_years, -1) - if self._is_weekday(dt): - self._add_holiday("Markets close at 2:00 PM ET (New Year's Day)", dt) @@ - # Day before Memorial Day (Friday before last Monday of May). - # Memorial Day is the last Monday of May. - memorial_day = _get_nth_weekday_of_month(-1, MON, MAY, self._year) - dt = _timedelta(memorial_day, -1) - if self._is_weekday(dt): - self._add_holiday("Markets close at 2:00 PM ET (Memorial Day)", dt)If you take Option A, please extend tests to include NYE and Memorial early-closes where applicable. If Option B, also trim the method doc comment listing these early-closes.
39-41: Consider category-specific start years (PUBLIC vs HALF_DAY).If HALF_DAY begins later than PUBLIC, add half_day_start_year and guard its population. Eases no_holidays tests and historical accuracy.
Based on learnings
13-17: Add minimal typing to satisfy Ruff (ANN002/ANN003/ANN204/ANN202).Annotate constructor and populate methods. Also import Any.
Apply:
@@ -from holidays.calendars.gregorian import _timedelta +from holidays.calendars.gregorian import _timedelta +from typing import Any @@ - def __init__(self, *args, **kwargs): + def __init__(self, *args: Any, **kwargs: Any) -> None: @@ - def _populate_public_holidays(self): + def _populate_public_holidays(self) -> None: @@ - def _populate_half_day_holidays(self): + def _populate_half_day_holidays(self) -> None:Also applies to: 39-47, 49-49, 98-98
tests/financial/test_us_government_securities.py (1)
38-41: Annotate setUpClass return type.Small typing tidy-up.
Apply:
@classmethod - def setUpClass(cls): + def setUpClass(cls) -> None: super().setUpClass(USGovernmentSecurities)holidays/financial/us_new_york.py (1)
48-57: Add return/param type hints.Minor typing polish; logic looks good.
Apply:
- def _populate_public_holidays(self): + def _populate_public_holidays(self) -> None: @@ - def _populate_half_day_holidays(self): + def _populate_half_day_holidays(self) -> None: @@ - def _remove_holiday_by_name(self, name): + def _remove_holiday_by_name(self, name: str) -> None:Also applies to: 58-64, 65-70
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (4)
holidays/financial/_sifma.py(1 hunks)holidays/financial/us_new_york.py(1 hunks)tests/financial/test_us_government_securities.py(1 hunks)tests/financial/test_us_new_york.py(1 hunks)
🧰 Additional context used
🧠 Learnings (19)
📚 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/financial/test_us_government_securities.pytests/financial/test_us_new_york.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/financial/test_us_government_securities.pytests/financial/test_us_new_york.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/financial/test_us_government_securities.pytests/financial/test_us_new_york.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:
holidays/financial/_sifma.py
📚 Learning: 2025-06-14T11:05:21.250Z
Learnt from: PPsyrius
PR: vacanza/holidays#2609
File: holidays/countries/nauru.py:48-50
Timestamp: 2025-06-14T11:05:21.250Z
Learning: In the holidays library, newer implementations use `start_year` to indicate the earliest year with complete holiday data coverage, not necessarily the first year a holiday existed. If a holiday system starts partway through a year (like Nauru's Public Holidays Act starting Jan 31, 1968), the start_year should be set to the following year (1969) to ensure users get full annual holiday coverage.
Applied to files:
holidays/financial/_sifma.py
📚 Learning: 2025-06-14T11:04:31.180Z
Learnt from: PPsyrius
PR: vacanza/holidays#2609
File: holidays/countries/nauru.py:57-60
Timestamp: 2025-06-14T11:04:31.180Z
Learning: In the holidays library, the base `HolidayBase._populate()` method already includes a guard clause that prevents holiday population methods like `_populate_public_holidays()` from being called when the year is before `start_year` or after `end_year`. Therefore, individual country implementations do not need to add their own guard clauses for years before independence or other start dates.
Applied to files:
holidays/financial/_sifma.py
📚 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/financial/_sifma.py
📚 Learning: 2025-08-26T14:43:53.605Z
Learnt from: KJhellico
PR: vacanza/holidays#2851
File: docs/holiday_categories.md:272-282
Timestamp: 2025-08-26T14:43:53.605Z
Learning: In the holidays library documentation, it's strongly advisable to recommend the use of constants from holidays.constants (e.g., PUBLIC, CATHOLIC) instead of direct string values when specifying holiday categories, as constants provide better type safety, IDE support, and prevent typos.
Applied to files:
holidays/financial/_sifma.py
📚 Learning: 2025-05-04T10:29:46.780Z
Learnt from: KJhellico
PR: vacanza/holidays#2525
File: holidays/countries/togo.py:0-0
Timestamp: 2025-05-04T10:29:46.780Z
Learning: When a country class in the holidays library uses additional categories beyond PUBLIC, the `supported_categories` tuple should contain all categories, including PUBLIC. Only when PUBLIC is the only category being used should it be omitted from `supported_categories`.
Applied to files:
holidays/financial/_sifma.py
📚 Learning: 2025-03-04T11:41:56.389Z
Learnt from: PPsyrius
PR: vacanza/holidays#2323
File: holidays/countries/macau.py:284-366
Timestamp: 2025-03-04T11:41:56.389Z
Learning: For Macau holidays implementation, maintaining separate methods for each holiday category (PUBLIC, MANDATORY, GOVERNMENT) is preferred because these categories are based on different sets of laws and have distinct historical evolution.
Applied to files:
holidays/financial/_sifma.py
📚 Learning: 2025-03-05T02:35:03.298Z
Learnt from: PPsyrius
PR: vacanza/holidays#2323
File: holidays/countries/macau.py:278-377
Timestamp: 2025-03-05T02:35:03.298Z
Learning: For Macau holiday implementations, it's preferable to maintain separate methods for different holiday categories (MANDATORY, GOVERNMENT, PUBLIC) as they are based on different sets of laws, making the code easier to maintain despite having multiple year-based conditionals.
Applied to files:
holidays/financial/_sifma.py
📚 Learning: 2025-03-04T11:32:45.095Z
Learnt from: PPsyrius
PR: vacanza/holidays#2323
File: holidays/countries/macau.py:407-461
Timestamp: 2025-03-04T11:32:45.095Z
Learning: In the holidays library, the standard approach for organizing static holidays is to use separate dictionaries for different categories (`government`, `mandatory`, and `public`), which are utilized by syntactic sugar methods.
Applied to files:
holidays/financial/_sifma.py
📚 Learning: 2025-06-18T10:58:28.058Z
Learnt from: PPsyrius
PR: vacanza/holidays#2642
File: holidays/countries/france.py:184-206
Timestamp: 2025-06-18T10:58:28.058Z
Learning: In the holidays library France implementation, creating shared private methods between subdivision-specific holiday population methods (like _populate_subdiv_57_public_holidays and _populate_subdiv_6ae_public_holidays) is not supported by the current framework architecture, so duplicate code between functionally identical subdivision methods should be left as-is.
Applied to files:
holidays/financial/_sifma.py
📚 Learning: 2025-08-28T02:42:52.755Z
Learnt from: PPsyrius
PR: vacanza/holidays#2863
File: tests/countries/test_georgia.py:31-36
Timestamp: 2025-08-28T02:42:52.755Z
Learning: In the holidays framework, when no categories parameter is specified in a country class instantiation (e.g., `Georgia(years=2025)`), the PUBLIC category is used by default. There's no need to explicitly specify `categories=PUBLIC` or import the PUBLIC constant for such test cases.
Applied to files:
holidays/financial/_sifma.py
📚 Learning: 2025-04-08T14:46:10.656Z
Learnt from: KJhellico
PR: vacanza/holidays#2437
File: holidays/countries/bhutan.py:27-30
Timestamp: 2025-04-08T14:46:10.656Z
Learning: For country classes in the holidays library, there's no need to explicitly specify `supported_categories = (PUBLIC,)` when PUBLIC is the only category being used, as it's already the default category inherited from HolidayBase.
Applied to files:
holidays/financial/_sifma.py
📚 Learning: 2025-03-04T11:32:45.095Z
Learnt from: PPsyrius
PR: vacanza/holidays#2323
File: holidays/countries/macau.py:407-461
Timestamp: 2025-03-04T11:32:45.095Z
Learning: In the holidays library, the standard approach for organizing static holidays is to use separate dictionaries for different categories (like `special_government_holidays`, `special_mandatory_holidays`, and `special_public_holidays`), which are utilized by syntactic sugar methods to pick up the appropriate holidays based on the selected category.
Applied to files:
holidays/financial/_sifma.py
📚 Learning: 2025-04-23T09:59:19.886Z
Learnt from: PPsyrius
PR: vacanza/holidays#2490
File: holidays/countries/ethiopia.py:45-45
Timestamp: 2025-04-23T09:59:19.886Z
Learning: For the Ethiopia holidays class, it's appropriate to add a return type hint only to the `_is_leap_year` method to match the base class implementation in `holidays/holiday_base.py`, while keeping other methods without type hints to maintain consistency with other country implementations.
Applied to files:
holidays/financial/us_new_york.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/financial/test_us_new_york.py
📚 Learning: 2025-09-03T16:49:35.246Z
Learnt from: PPsyrius
PR: vacanza/holidays#2881
File: tests/countries/test_argentina.py:375-375
Timestamp: 2025-09-03T16:49:35.246Z
Learning: In the holidays library, national holiday tests use self.full_range (or similar comprehensive year ranges) even when explicit test dates only show modern observance. This is intentional for correctness and comprehensive coverage, unlike subdivision-specific holidays which have explicit year boundaries based on known start dates.
Applied to files:
tests/financial/test_us_new_york.py
🧬 Code graph analysis (4)
tests/financial/test_us_government_securities.py (3)
tests/common.py (5)
assertAliases(124-133)assertNoHolidays(324-326)assertHoliday(153-155)assertNoHoliday(276-278)assertHolidays(231-233)holidays/calendars/gregorian.py (1)
_timedelta(37-42)holidays/financial/us_government_securities.py (3)
USGovernmentSecurities(16-39)USGS(42-43)USBondMarket(46-47)
holidays/financial/_sifma.py (5)
holidays/calendars/gregorian.py (2)
_timedelta(37-42)_get_nth_weekday_of_month(64-89)holidays/groups/christian.py (4)
_add_good_friday(344-353)_add_christmas_day(224-232)_easter_sunday(105-109)_christmas_day(98-102)holidays/groups/international.py (2)
_add_new_years_day(142-150)_add_remembrance_day(182-190)holidays/observed_holiday_base.py (1)
_move_holiday(199-211)holidays/holiday_base.py (2)
_is_weekday(868-873)_add_holiday(797-809)
holidays/financial/us_new_york.py (2)
holidays/financial/_sifma.py (3)
SIFMAHolidays(19-156)_populate_public_holidays(49-96)_populate_half_day_holidays(98-156)holidays/holiday_base.py (1)
pop(1173-1202)
tests/financial/test_us_new_york.py (3)
tests/common.py (6)
TestCase(31-370)assertAliases(124-133)assertNoHolidays(324-326)assertHoliday(153-155)assertNoHoliday(276-278)assertHolidays(231-233)holidays/calendars/gregorian.py (1)
_timedelta(37-42)holidays/financial/us_new_york.py (3)
USNewYork(16-69)USNY(72-73)USNewYorkFinancial(76-77)
🪛 Ruff (0.14.1)
tests/financial/test_us_government_securities.py
40-40: Missing return type annotation for classmethod setUpClass
Add return type annotation: None
(ANN206)
holidays/financial/_sifma.py
43-43: Missing return type annotation for special method __init__
Add return type annotation: None
(ANN204)
43-43: Missing type annotation for *args
(ANN002)
43-43: Missing type annotation for **kwargs
(ANN003)
49-49: Missing return type annotation for private function _populate_public_holidays
Add return type annotation: None
(ANN202)
98-98: Missing return type annotation for private function _populate_half_day_holidays
Add return type annotation: None
(ANN202)
holidays/financial/us_new_york.py
48-48: Missing return type annotation for private function _populate_public_holidays
Add return type annotation: None
(ANN202)
58-58: Missing return type annotation for private function _populate_half_day_holidays
Add return type annotation: None
(ANN202)
65-65: Missing return type annotation for private function _remove_holiday_by_name
Add return type annotation: None
(ANN202)
tests/financial/test_us_new_york.py
24-24: Missing return type annotation for classmethod setUpClass
Add return type annotation: None
(ANN206)
🔇 Additional comments (1)
tests/financial/test_us_government_securities.py (1)
245-268: If you adopt robust HALF_DAY (incl. NYE/Memorial), update expectations.Option A in SIFMA adds NYE/Memorial early‑closes; extend these assertions accordingly for affected years.
Also applies to: 269-294, 295-317
KJhellico
reviewed
Oct 21, 2025
- Move imports to top of file (no local imports) - Use auto-generated helper methods instead of manual calculations: * _add_holy_thursday for Maundy Thursday * _add_holiday_3_days_prior_last_mon_of_may for Memorial Day * _add_holiday_1_day_past_4th_thu_of_nov for Thanksgiving - Fix Independence Day and Christmas early close logic to check Tue-Fri - Remove excessive comments - Update tests to match new HALF_DAY dates (now includes Memorial Day early close) 🤖 Generated with [Claude Code](https://claude.com/claude-code)
Co-authored-by: ~Jhellico <KJhellico@users.noreply.github.com> Signed-off-by: Ryley Mao <rymao_@outlook.com>
Co-authored-by: ~Jhellico <KJhellico@users.noreply.github.com> Signed-off-by: Ryley Mao <rymao_@outlook.com>
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
holidays/financial/_sifma.py (1)
122-156: Consider using a label variable for early-close times.The "Markets close at 2:00 PM ET" string is repeated multiple times. Extract it to a variable for consistency and maintainability, as suggested in earlier reviews.
def _populate_half_day_holidays(self): + begin_time_label = "Markets close at 2:00 PM ET (%s)" + # Day before New Year's Day. - self._add_holiday("Markets close at 2:00 PM ET (New Year's Day)", early_close_nye) + self._add_holiday(begin_time_label % "New Year's Day", early_close_nye) # Day before Good Friday (Maundy Thursday). - self._add_holy_thursday("Markets close at 2:00 PM ET (Good Friday)") + self._add_holy_thursday(begin_time_label % "Good Friday") # Friday before Memorial Day. - self._add_holiday_3_days_prior_last_mon_of_may("Markets close at 2:00 PM ET (Memorial Day)") + self._add_holiday_3_days_prior_last_mon_of_may(begin_time_label % "Memorial Day") - self._add_holiday("Markets close at 2:00 PM ET (Memorial Day)", early_close_memorial) + self._add_holiday(begin_time_label % "Memorial Day", early_close_memorial) # Day before Independence Day. - self._add_holiday("Markets close at 2:00 PM ET (Independence Day)", early_close_jul_4) + self._add_holiday(begin_time_label % "Independence Day", early_close_jul_4) # Day after Thanksgiving (Black Friday). - self._add_holiday_1_day_past_4th_thu_of_nov("Markets close at 2:00 PM ET (Thanksgiving Day)") + self._add_holiday_1_day_past_4th_thu_of_nov(begin_time_label % "Thanksgiving Day") # Day before Christmas. - self._add_holiday("Markets close at 2:00 PM ET (Christmas Day)", early_close_christmas) + self._add_holiday(begin_time_label % "Christmas Day", early_close_christmas)
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (1)
holidays/financial/_sifma.py(1 hunks)
🧰 Additional context used
🧠 Learnings (31)
📚 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:
holidays/financial/_sifma.py
📚 Learning: 2025-06-14T11:05:21.250Z
Learnt from: PPsyrius
PR: vacanza/holidays#2609
File: holidays/countries/nauru.py:48-50
Timestamp: 2025-06-14T11:05:21.250Z
Learning: In the holidays library, newer implementations use `start_year` to indicate the earliest year with complete holiday data coverage, not necessarily the first year a holiday existed. If a holiday system starts partway through a year (like Nauru's Public Holidays Act starting Jan 31, 1968), the start_year should be set to the following year (1969) to ensure users get full annual holiday coverage.
Applied to files:
holidays/financial/_sifma.py
📚 Learning: 2025-06-14T11:04:31.180Z
Learnt from: PPsyrius
PR: vacanza/holidays#2609
File: holidays/countries/nauru.py:57-60
Timestamp: 2025-06-14T11:04:31.180Z
Learning: In the holidays library, the base `HolidayBase._populate()` method already includes a guard clause that prevents holiday population methods like `_populate_public_holidays()` from being called when the year is before `start_year` or after `end_year`. Therefore, individual country implementations do not need to add their own guard clauses for years before independence or other start dates.
Applied to files:
holidays/financial/_sifma.py
📚 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/financial/_sifma.py
📚 Learning: 2025-08-26T14:43:53.605Z
Learnt from: KJhellico
PR: vacanza/holidays#2851
File: docs/holiday_categories.md:272-282
Timestamp: 2025-08-26T14:43:53.605Z
Learning: In the holidays library documentation, it's strongly advisable to recommend the use of constants from holidays.constants (e.g., PUBLIC, CATHOLIC) instead of direct string values when specifying holiday categories, as constants provide better type safety, IDE support, and prevent typos.
Applied to files:
holidays/financial/_sifma.py
📚 Learning: 2025-05-04T10:29:46.780Z
Learnt from: KJhellico
PR: vacanza/holidays#2525
File: holidays/countries/togo.py:0-0
Timestamp: 2025-05-04T10:29:46.780Z
Learning: When a country class in the holidays library uses additional categories beyond PUBLIC, the `supported_categories` tuple should contain all categories, including PUBLIC. Only when PUBLIC is the only category being used should it be omitted from `supported_categories`.
Applied to files:
holidays/financial/_sifma.py
📚 Learning: 2025-03-04T11:41:56.389Z
Learnt from: PPsyrius
PR: vacanza/holidays#2323
File: holidays/countries/macau.py:284-366
Timestamp: 2025-03-04T11:41:56.389Z
Learning: For Macau holidays implementation, maintaining separate methods for each holiday category (PUBLIC, MANDATORY, GOVERNMENT) is preferred because these categories are based on different sets of laws and have distinct historical evolution.
Applied to files:
holidays/financial/_sifma.py
📚 Learning: 2025-03-05T02:35:03.298Z
Learnt from: PPsyrius
PR: vacanza/holidays#2323
File: holidays/countries/macau.py:278-377
Timestamp: 2025-03-05T02:35:03.298Z
Learning: For Macau holiday implementations, it's preferable to maintain separate methods for different holiday categories (MANDATORY, GOVERNMENT, PUBLIC) as they are based on different sets of laws, making the code easier to maintain despite having multiple year-based conditionals.
Applied to files:
holidays/financial/_sifma.py
📚 Learning: 2025-03-04T11:32:45.095Z
Learnt from: PPsyrius
PR: vacanza/holidays#2323
File: holidays/countries/macau.py:407-461
Timestamp: 2025-03-04T11:32:45.095Z
Learning: In the holidays library, the standard approach for organizing static holidays is to use separate dictionaries for different categories (`government`, `mandatory`, and `public`), which are utilized by syntactic sugar methods.
Applied to files:
holidays/financial/_sifma.py
📚 Learning: 2025-06-18T10:58:28.058Z
Learnt from: PPsyrius
PR: vacanza/holidays#2642
File: holidays/countries/france.py:184-206
Timestamp: 2025-06-18T10:58:28.058Z
Learning: In the holidays library France implementation, creating shared private methods between subdivision-specific holiday population methods (like _populate_subdiv_57_public_holidays and _populate_subdiv_6ae_public_holidays) is not supported by the current framework architecture, so duplicate code between functionally identical subdivision methods should be left as-is.
Applied to files:
holidays/financial/_sifma.py
📚 Learning: 2025-08-28T02:42:52.755Z
Learnt from: PPsyrius
PR: vacanza/holidays#2863
File: tests/countries/test_georgia.py:31-36
Timestamp: 2025-08-28T02:42:52.755Z
Learning: In the holidays framework, when no categories parameter is specified in a country class instantiation (e.g., `Georgia(years=2025)`), the PUBLIC category is used by default. There's no need to explicitly specify `categories=PUBLIC` or import the PUBLIC constant for such test cases.
Applied to files:
holidays/financial/_sifma.py
📚 Learning: 2025-04-08T14:46:10.656Z
Learnt from: KJhellico
PR: vacanza/holidays#2437
File: holidays/countries/bhutan.py:27-30
Timestamp: 2025-04-08T14:46:10.656Z
Learning: For country classes in the holidays library, there's no need to explicitly specify `supported_categories = (PUBLIC,)` when PUBLIC is the only category being used, as it's already the default category inherited from HolidayBase.
Applied to files:
holidays/financial/_sifma.py
📚 Learning: 2025-03-04T11:32:45.095Z
Learnt from: PPsyrius
PR: vacanza/holidays#2323
File: holidays/countries/macau.py:407-461
Timestamp: 2025-03-04T11:32:45.095Z
Learning: In the holidays library, the standard approach for organizing static holidays is to use separate dictionaries for different categories (like `special_government_holidays`, `special_mandatory_holidays`, and `special_public_holidays`), which are utilized by syntactic sugar methods to pick up the appropriate holidays based on the selected category.
Applied to files:
holidays/financial/_sifma.py
📚 Learning: 2025-06-18T10:21:01.376Z
Learnt from: PPsyrius
PR: vacanza/holidays#2642
File: holidays/countries/france.py:300-319
Timestamp: 2025-06-18T10:21:01.376Z
Learning: In the France holidays implementation, legislative years for holiday changes should be hard-coded rather than extracted into constants, as this maintains consistency with the existing codebase pattern and provides historical accuracy for specific legislative acts.
Applied to files:
holidays/financial/_sifma.py
📚 Learning: 2025-07-14T20:23:48.198Z
Learnt from: KJhellico
PR: vacanza/holidays#2654
File: holidays/countries/cabo_verde.py:133-141
Timestamp: 2025-07-14T20:23:48.198Z
Learning: The holidays library provides helper methods `_add_holiday_2nd_sun_of_may()` and `_add_holiday_3rd_sun_of_jun()` for adding holidays on the 2nd Sunday of May and 3rd Sunday of June respectively. These methods are used across multiple country implementations including Latvia, Finland, Belarus, Malaysia, Madagascar, and Cape Verde.
Applied to files:
holidays/financial/_sifma.py
📚 Learning: 2025-05-13T13:23:11.375Z
Learnt from: KJhellico
PR: vacanza/holidays#2483
File: holidays/countries/turks_and_caicos_islands.py:117-118
Timestamp: 2025-05-13T13:23:11.375Z
Learning: The holidays library uses `_add_christmas_day_two` method to add Boxing Day holiday, not `_add_boxing_day`.
Applied to files:
holidays/financial/_sifma.py
📚 Learning: 2025-06-14T20:43:15.370Z
Learnt from: KJhellico
PR: vacanza/holidays#2631
File: holidays/countries/sint_maarten.py:94-95
Timestamp: 2025-06-14T20:43:15.370Z
Learning: The `_add_*` helper methods in the holidays library (such as `_add_christmas_day_two()`, `_add_labor_day()`, etc.) don't have default holiday names, so the name parameter should always be explicitly specified when calling these methods.
Applied to files:
holidays/financial/_sifma.py
📚 Learning: 2025-04-03T16:58:27.175Z
Learnt from: Wasif-Shahzad
PR: vacanza/holidays#2409
File: holidays/countries/qatar.py:27-46
Timestamp: 2025-04-03T16:58:27.175Z
Learning: In the holidays library, method names like `_add_holiday_2nd_tue_of_feb()` and `_add_holiday_1st_sun_of_mar()` use calendar constants like FEB, TUE, MAR, and SUN internally through parent class implementations even when these constants don't appear directly in the file.
Applied to files:
holidays/financial/_sifma.py
📚 Learning: 2025-08-19T21:00:47.849Z
Learnt from: KJhellico
PR: vacanza/holidays#2831
File: holidays/countries/south_sudan.py:84-88
Timestamp: 2025-08-19T21:00:47.849Z
Learning: In the holidays library, Islamic holidays use dedicated methods for additional days (like `_add_eid_al_fitr_day_two`, `_add_eid_al_adha_day_two`) rather than parameters. The methods don't accept a `days` parameter - each day has its own specific method.
Applied to files:
holidays/financial/_sifma.py
📚 Learning: 2025-07-09T21:16:35.145Z
Learnt from: KJhellico
PR: vacanza/holidays#2623
File: tests/countries/test_christmas_island.py:136-146
Timestamp: 2025-07-09T21:16:35.145Z
Learning: In Christmas Island's holiday implementation, the test_christmas_day method cannot use assertNoNonObservedHoliday because in some years observed Christmas Day overlaps with Boxing Day when both holidays are moved due to weekend conflicts, causing the standard non-observed holiday check to fail inappropriately.
Applied to files:
holidays/financial/_sifma.py
📚 Learning: 2025-06-16T12:28:31.641Z
Learnt from: PPsyrius
PR: vacanza/holidays#2632
File: holidays/countries/solomon_islands.py:95-98
Timestamp: 2025-06-16T12:28:31.641Z
Learning: Library-wide holiday patterns and their optimizations should be handled at the base class level (like InternationalHolidays) rather than documenting workarounds in individual country modules. This maintains separation of concerns and avoids documentation duplication.
Applied to files:
holidays/financial/_sifma.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:
holidays/financial/_sifma.py
📚 Learning: 2025-09-12T21:37:10.710Z
Learnt from: KJhellico
PR: vacanza/holidays#2854
File: tests/countries/test_sudan.py:29-31
Timestamp: 2025-09-12T21:37:10.710Z
Learning: For countries in the holidays library that only use the default PUBLIC category (like Sudan), there's no need to specify `categories=Sudan.supported_categories` in test_no_holidays methods, as it would be equivalent to the default behavior.
Applied to files:
holidays/financial/_sifma.py
📚 Learning: 2025-09-14T16:19:23.651Z
Learnt from: PPsyrius
PR: vacanza/holidays#2881
File: tests/countries/test_antigua_and_barbuda.py:27-29
Timestamp: 2025-09-14T16:19:23.651Z
Learning: For Antigua and Barbuda in the holidays library, only the default PUBLIC category is used, so there's no need to specify `categories=AntiguaAndBarbuda.supported_categories` in test_no_holidays methods, as it would be equivalent to the default behavior and AntiguaAndBarbuda doesn't define supported_categories.
Applied to files:
holidays/financial/_sifma.py
📚 Learning: 2025-09-14T16:02:15.480Z
Learnt from: PPsyrius
PR: vacanza/holidays#2881
File: tests/countries/test_bahamas.py:27-29
Timestamp: 2025-09-14T16:02:15.480Z
Learning: For Bahamas in the holidays library, only the default PUBLIC category is used, so there's no need to specify `categories=Bahamas.supported_categories` in test_no_holidays methods, as it would be equivalent to the default behavior.
Applied to files:
holidays/financial/_sifma.py
📚 Learning: 2025-08-03T13:48:11.910Z
Learnt from: KJhellico
PR: vacanza/holidays#2777
File: holidays/countries/gambia.py:120-122
Timestamp: 2025-08-03T13:48:11.910Z
Learning: When reviewing holiday implementations in the holidays library, defer to the maintainers' choice of start years for specific holiday policies, as they likely have access to more reliable primary sources and official documentation than what can be found through web searches.
Applied to files:
holidays/financial/_sifma.py
📚 Learning: 2025-10-21T09:59:18.714Z
Learnt from: KJhellico
PR: vacanza/holidays#3008
File: tests/financial/test_us_new_york.py:22-26
Timestamp: 2025-10-21T09:59:18.714Z
Learning: In the holidays (vacanza) project, do not suggest adding return type annotations. The project will address return type annotations comprehensively in the future rather than incrementally.
Applied to files:
holidays/financial/_sifma.py
📚 Learning: 2025-04-23T09:22:41.753Z
Learnt from: PPsyrius
PR: vacanza/holidays#2489
File: holidays/countries/sao_tome_and_principe.py:86-88
Timestamp: 2025-04-23T09:22:41.753Z
Learning: For holiday definitions in the holidays package, keep comments simple with just the holiday name (e.g., "# Independence Day.") rather than including dates or historical context, as the function names already encode the date information.
Applied to files:
holidays/financial/_sifma.py
📚 Learning: 2025-08-19T21:22:13.125Z
Learnt from: KJhellico
PR: vacanza/holidays#2831
File: holidays/countries/south_sudan.py:84-88
Timestamp: 2025-08-19T21:22:13.125Z
Learning: In the holidays library, message comments directly above holiday addition methods must match the holiday name exactly without any additions, modifications, or explanatory text. For example, if the holiday name is "Eid al-Fitr Holiday", the comment should be "# Eid al-Fitr Holiday." with no extra context.
Applied to files:
holidays/financial/_sifma.py
📚 Learning: 2025-05-10T04:02:13.815Z
Learnt from: PPsyrius
PR: vacanza/holidays#2537
File: holidays/countries/finland.py:249-253
Timestamp: 2025-05-10T04:02:13.815Z
Learning: Holiday name comments directly above tr() function calls in the holidays package should only contain the holiday name itself (e.g., "# Independence Day.") without any additional context, dates, or historical information.
Applied to files:
holidays/financial/_sifma.py
📚 Learning: 2025-05-10T04:32:15.760Z
Learnt from: PPsyrius
PR: vacanza/holidays#2537
File: holidays/countries/finland.py:0-0
Timestamp: 2025-05-10T04:32:15.760Z
Learning: In the holidays package, detailed historical context and additional information should be added as comments at the method level or above conditional blocks, while comments directly above tr() function calls should only contain the holiday name itself (e.g., "# Independence Day.").
Applied to files:
holidays/financial/_sifma.py
🧬 Code graph analysis (1)
holidays/financial/_sifma.py (5)
holidays/groups/christian.py (3)
_add_good_friday(344-353)_add_christmas_day(224-232)_add_holy_thursday(364-373)holidays/groups/international.py (2)
_add_new_years_day(142-150)_add_remembrance_day(182-190)holidays/observed_holiday_base.py (4)
ObservedHolidayBase(102-256)ObservedRule(20-24)_move_holiday(199-211)_get_observed_date(130-142)holidays/financial/us_new_york.py (2)
_populate_public_holidays(48-56)_populate_half_day_holidays(58-63)holidays/holiday_base.py (1)
_add_holiday(797-809)
🪛 Ruff (0.14.1)
holidays/financial/_sifma.py
48-48: Missing return type annotation for special method __init__
Add return type annotation: None
(ANN204)
48-48: Missing type annotation for *args
(ANN002)
48-48: Missing type annotation for **kwargs
(ANN003)
54-54: Missing return type annotation for private function _populate_public_holidays
Add return type annotation: None
(ANN202)
122-122: Missing return type annotation for private function _populate_half_day_holidays
Add return type annotation: None
(ANN202)
🔇 Additional comments (1)
holidays/financial/_sifma.py (1)
23-23: SIFMA_EARLY_CLOSE rule looks correct.The observed rule correctly handles early-close logic for all weekday scenarios. The MON: -3 and SUN: -2 mappings ensure Friday early closes when holidays fall on Sunday or Monday, which matches SIFMA recommendations.
- Add missing Day before New Year's Day early close to SIFMA base class - Remove SIFMA_EARLY_CLOSE comment block as requested - Update References section with archived URLs and date ranges - Simplify holiday comments (remove historical details except Good Friday note) - Update all test files to include New Year's Eve early close dates for USGS and USNY Addresses KJhellico's code review comments and CodeRabbitAI bot feedback
- Remove verbose historical explanations from holiday comments - Add begin_time_label variable for DRY early close names - Use string formatting with % operator for all early close holidays Addresses remaining KJhellico code review suggestions
Remove redundant year check in New Year's Eve early close logic. The SIFMA_EARLY_CLOSE rule with negative offsets always places the early close in the current year's December when applied to next year's January 1.
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (1)
holidays/financial/_sifma.py(1 hunks)
🧰 Additional context used
🧠 Learnings (32)
📚 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:
holidays/financial/_sifma.py
📚 Learning: 2025-06-14T11:05:21.250Z
Learnt from: PPsyrius
PR: vacanza/holidays#2609
File: holidays/countries/nauru.py:48-50
Timestamp: 2025-06-14T11:05:21.250Z
Learning: In the holidays library, newer implementations use `start_year` to indicate the earliest year with complete holiday data coverage, not necessarily the first year a holiday existed. If a holiday system starts partway through a year (like Nauru's Public Holidays Act starting Jan 31, 1968), the start_year should be set to the following year (1969) to ensure users get full annual holiday coverage.
Applied to files:
holidays/financial/_sifma.py
📚 Learning: 2025-06-14T11:04:31.180Z
Learnt from: PPsyrius
PR: vacanza/holidays#2609
File: holidays/countries/nauru.py:57-60
Timestamp: 2025-06-14T11:04:31.180Z
Learning: In the holidays library, the base `HolidayBase._populate()` method already includes a guard clause that prevents holiday population methods like `_populate_public_holidays()` from being called when the year is before `start_year` or after `end_year`. Therefore, individual country implementations do not need to add their own guard clauses for years before independence or other start dates.
Applied to files:
holidays/financial/_sifma.py
📚 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/financial/_sifma.py
📚 Learning: 2025-08-26T14:43:53.605Z
Learnt from: KJhellico
PR: vacanza/holidays#2851
File: docs/holiday_categories.md:272-282
Timestamp: 2025-08-26T14:43:53.605Z
Learning: In the holidays library documentation, it's strongly advisable to recommend the use of constants from holidays.constants (e.g., PUBLIC, CATHOLIC) instead of direct string values when specifying holiday categories, as constants provide better type safety, IDE support, and prevent typos.
Applied to files:
holidays/financial/_sifma.py
📚 Learning: 2025-05-04T10:29:46.780Z
Learnt from: KJhellico
PR: vacanza/holidays#2525
File: holidays/countries/togo.py:0-0
Timestamp: 2025-05-04T10:29:46.780Z
Learning: When a country class in the holidays library uses additional categories beyond PUBLIC, the `supported_categories` tuple should contain all categories, including PUBLIC. Only when PUBLIC is the only category being used should it be omitted from `supported_categories`.
Applied to files:
holidays/financial/_sifma.py
📚 Learning: 2025-03-04T11:41:56.389Z
Learnt from: PPsyrius
PR: vacanza/holidays#2323
File: holidays/countries/macau.py:284-366
Timestamp: 2025-03-04T11:41:56.389Z
Learning: For Macau holidays implementation, maintaining separate methods for each holiday category (PUBLIC, MANDATORY, GOVERNMENT) is preferred because these categories are based on different sets of laws and have distinct historical evolution.
Applied to files:
holidays/financial/_sifma.py
📚 Learning: 2025-03-05T02:35:03.298Z
Learnt from: PPsyrius
PR: vacanza/holidays#2323
File: holidays/countries/macau.py:278-377
Timestamp: 2025-03-05T02:35:03.298Z
Learning: For Macau holiday implementations, it's preferable to maintain separate methods for different holiday categories (MANDATORY, GOVERNMENT, PUBLIC) as they are based on different sets of laws, making the code easier to maintain despite having multiple year-based conditionals.
Applied to files:
holidays/financial/_sifma.py
📚 Learning: 2025-03-04T11:32:45.095Z
Learnt from: PPsyrius
PR: vacanza/holidays#2323
File: holidays/countries/macau.py:407-461
Timestamp: 2025-03-04T11:32:45.095Z
Learning: In the holidays library, the standard approach for organizing static holidays is to use separate dictionaries for different categories (`government`, `mandatory`, and `public`), which are utilized by syntactic sugar methods.
Applied to files:
holidays/financial/_sifma.py
📚 Learning: 2025-06-18T10:58:28.058Z
Learnt from: PPsyrius
PR: vacanza/holidays#2642
File: holidays/countries/france.py:184-206
Timestamp: 2025-06-18T10:58:28.058Z
Learning: In the holidays library France implementation, creating shared private methods between subdivision-specific holiday population methods (like _populate_subdiv_57_public_holidays and _populate_subdiv_6ae_public_holidays) is not supported by the current framework architecture, so duplicate code between functionally identical subdivision methods should be left as-is.
Applied to files:
holidays/financial/_sifma.py
📚 Learning: 2025-08-28T02:42:52.755Z
Learnt from: PPsyrius
PR: vacanza/holidays#2863
File: tests/countries/test_georgia.py:31-36
Timestamp: 2025-08-28T02:42:52.755Z
Learning: In the holidays framework, when no categories parameter is specified in a country class instantiation (e.g., `Georgia(years=2025)`), the PUBLIC category is used by default. There's no need to explicitly specify `categories=PUBLIC` or import the PUBLIC constant for such test cases.
Applied to files:
holidays/financial/_sifma.py
📚 Learning: 2025-04-08T14:46:10.656Z
Learnt from: KJhellico
PR: vacanza/holidays#2437
File: holidays/countries/bhutan.py:27-30
Timestamp: 2025-04-08T14:46:10.656Z
Learning: For country classes in the holidays library, there's no need to explicitly specify `supported_categories = (PUBLIC,)` when PUBLIC is the only category being used, as it's already the default category inherited from HolidayBase.
Applied to files:
holidays/financial/_sifma.py
📚 Learning: 2025-03-04T11:32:45.095Z
Learnt from: PPsyrius
PR: vacanza/holidays#2323
File: holidays/countries/macau.py:407-461
Timestamp: 2025-03-04T11:32:45.095Z
Learning: In the holidays library, the standard approach for organizing static holidays is to use separate dictionaries for different categories (like `special_government_holidays`, `special_mandatory_holidays`, and `special_public_holidays`), which are utilized by syntactic sugar methods to pick up the appropriate holidays based on the selected category.
Applied to files:
holidays/financial/_sifma.py
📚 Learning: 2025-06-18T10:21:01.376Z
Learnt from: PPsyrius
PR: vacanza/holidays#2642
File: holidays/countries/france.py:300-319
Timestamp: 2025-06-18T10:21:01.376Z
Learning: In the France holidays implementation, legislative years for holiday changes should be hard-coded rather than extracted into constants, as this maintains consistency with the existing codebase pattern and provides historical accuracy for specific legislative acts.
Applied to files:
holidays/financial/_sifma.py
📚 Learning: 2025-07-14T20:23:48.198Z
Learnt from: KJhellico
PR: vacanza/holidays#2654
File: holidays/countries/cabo_verde.py:133-141
Timestamp: 2025-07-14T20:23:48.198Z
Learning: The holidays library provides helper methods `_add_holiday_2nd_sun_of_may()` and `_add_holiday_3rd_sun_of_jun()` for adding holidays on the 2nd Sunday of May and 3rd Sunday of June respectively. These methods are used across multiple country implementations including Latvia, Finland, Belarus, Malaysia, Madagascar, and Cape Verde.
Applied to files:
holidays/financial/_sifma.py
📚 Learning: 2025-05-13T13:23:11.375Z
Learnt from: KJhellico
PR: vacanza/holidays#2483
File: holidays/countries/turks_and_caicos_islands.py:117-118
Timestamp: 2025-05-13T13:23:11.375Z
Learning: The holidays library uses `_add_christmas_day_two` method to add Boxing Day holiday, not `_add_boxing_day`.
Applied to files:
holidays/financial/_sifma.py
📚 Learning: 2025-06-14T20:43:15.370Z
Learnt from: KJhellico
PR: vacanza/holidays#2631
File: holidays/countries/sint_maarten.py:94-95
Timestamp: 2025-06-14T20:43:15.370Z
Learning: The `_add_*` helper methods in the holidays library (such as `_add_christmas_day_two()`, `_add_labor_day()`, etc.) don't have default holiday names, so the name parameter should always be explicitly specified when calling these methods.
Applied to files:
holidays/financial/_sifma.py
📚 Learning: 2025-04-03T16:58:27.175Z
Learnt from: Wasif-Shahzad
PR: vacanza/holidays#2409
File: holidays/countries/qatar.py:27-46
Timestamp: 2025-04-03T16:58:27.175Z
Learning: In the holidays library, method names like `_add_holiday_2nd_tue_of_feb()` and `_add_holiday_1st_sun_of_mar()` use calendar constants like FEB, TUE, MAR, and SUN internally through parent class implementations even when these constants don't appear directly in the file.
Applied to files:
holidays/financial/_sifma.py
📚 Learning: 2025-08-19T21:00:47.849Z
Learnt from: KJhellico
PR: vacanza/holidays#2831
File: holidays/countries/south_sudan.py:84-88
Timestamp: 2025-08-19T21:00:47.849Z
Learning: In the holidays library, Islamic holidays use dedicated methods for additional days (like `_add_eid_al_fitr_day_two`, `_add_eid_al_adha_day_two`) rather than parameters. The methods don't accept a `days` parameter - each day has its own specific method.
Applied to files:
holidays/financial/_sifma.py
📚 Learning: 2025-07-09T21:16:35.145Z
Learnt from: KJhellico
PR: vacanza/holidays#2623
File: tests/countries/test_christmas_island.py:136-146
Timestamp: 2025-07-09T21:16:35.145Z
Learning: In Christmas Island's holiday implementation, the test_christmas_day method cannot use assertNoNonObservedHoliday because in some years observed Christmas Day overlaps with Boxing Day when both holidays are moved due to weekend conflicts, causing the standard non-observed holiday check to fail inappropriately.
Applied to files:
holidays/financial/_sifma.py
📚 Learning: 2025-06-16T12:28:31.641Z
Learnt from: PPsyrius
PR: vacanza/holidays#2632
File: holidays/countries/solomon_islands.py:95-98
Timestamp: 2025-06-16T12:28:31.641Z
Learning: Library-wide holiday patterns and their optimizations should be handled at the base class level (like InternationalHolidays) rather than documenting workarounds in individual country modules. This maintains separation of concerns and avoids documentation duplication.
Applied to files:
holidays/financial/_sifma.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:
holidays/financial/_sifma.py
📚 Learning: 2025-09-12T21:37:10.710Z
Learnt from: KJhellico
PR: vacanza/holidays#2854
File: tests/countries/test_sudan.py:29-31
Timestamp: 2025-09-12T21:37:10.710Z
Learning: For countries in the holidays library that only use the default PUBLIC category (like Sudan), there's no need to specify `categories=Sudan.supported_categories` in test_no_holidays methods, as it would be equivalent to the default behavior.
Applied to files:
holidays/financial/_sifma.py
📚 Learning: 2025-09-14T16:19:23.651Z
Learnt from: PPsyrius
PR: vacanza/holidays#2881
File: tests/countries/test_antigua_and_barbuda.py:27-29
Timestamp: 2025-09-14T16:19:23.651Z
Learning: For Antigua and Barbuda in the holidays library, only the default PUBLIC category is used, so there's no need to specify `categories=AntiguaAndBarbuda.supported_categories` in test_no_holidays methods, as it would be equivalent to the default behavior and AntiguaAndBarbuda doesn't define supported_categories.
Applied to files:
holidays/financial/_sifma.py
📚 Learning: 2025-09-14T16:02:15.480Z
Learnt from: PPsyrius
PR: vacanza/holidays#2881
File: tests/countries/test_bahamas.py:27-29
Timestamp: 2025-09-14T16:02:15.480Z
Learning: For Bahamas in the holidays library, only the default PUBLIC category is used, so there's no need to specify `categories=Bahamas.supported_categories` in test_no_holidays methods, as it would be equivalent to the default behavior.
Applied to files:
holidays/financial/_sifma.py
📚 Learning: 2025-08-03T13:48:11.910Z
Learnt from: KJhellico
PR: vacanza/holidays#2777
File: holidays/countries/gambia.py:120-122
Timestamp: 2025-08-03T13:48:11.910Z
Learning: When reviewing holiday implementations in the holidays library, defer to the maintainers' choice of start years for specific holiday policies, as they likely have access to more reliable primary sources and official documentation than what can be found through web searches.
Applied to files:
holidays/financial/_sifma.py
📚 Learning: 2025-10-21T09:59:18.714Z
Learnt from: KJhellico
PR: vacanza/holidays#3008
File: tests/financial/test_us_new_york.py:22-26
Timestamp: 2025-10-21T09:59:18.714Z
Learning: In the holidays (vacanza) project, do not suggest adding return type annotations. The project will address return type annotations comprehensively in the future rather than incrementally.
Applied to files:
holidays/financial/_sifma.py
📚 Learning: 2025-04-23T09:22:41.753Z
Learnt from: PPsyrius
PR: vacanza/holidays#2489
File: holidays/countries/sao_tome_and_principe.py:86-88
Timestamp: 2025-04-23T09:22:41.753Z
Learning: For holiday definitions in the holidays package, keep comments simple with just the holiday name (e.g., "# Independence Day.") rather than including dates or historical context, as the function names already encode the date information.
Applied to files:
holidays/financial/_sifma.py
📚 Learning: 2025-08-19T21:22:13.125Z
Learnt from: KJhellico
PR: vacanza/holidays#2831
File: holidays/countries/south_sudan.py:84-88
Timestamp: 2025-08-19T21:22:13.125Z
Learning: In the holidays library, message comments directly above holiday addition methods must match the holiday name exactly without any additions, modifications, or explanatory text. For example, if the holiday name is "Eid al-Fitr Holiday", the comment should be "# Eid al-Fitr Holiday." with no extra context.
Applied to files:
holidays/financial/_sifma.py
📚 Learning: 2025-05-10T04:02:13.815Z
Learnt from: PPsyrius
PR: vacanza/holidays#2537
File: holidays/countries/finland.py:249-253
Timestamp: 2025-05-10T04:02:13.815Z
Learning: Holiday name comments directly above tr() function calls in the holidays package should only contain the holiday name itself (e.g., "# Independence Day.") without any additional context, dates, or historical information.
Applied to files:
holidays/financial/_sifma.py
📚 Learning: 2025-05-10T04:32:15.760Z
Learnt from: PPsyrius
PR: vacanza/holidays#2537
File: holidays/countries/finland.py:0-0
Timestamp: 2025-05-10T04:32:15.760Z
Learning: In the holidays package, detailed historical context and additional information should be added as comments at the method level or above conditional blocks, while comments directly above tr() function calls should only contain the holiday name itself (e.g., "# Independence Day.").
Applied to files:
holidays/financial/_sifma.py
📚 Learning: 2025-07-02T18:17:53.342Z
Learnt from: KJhellico
PR: vacanza/holidays#2608
File: tests/countries/test_saint_vincent_and_the_grenadines.py:162-178
Timestamp: 2025-07-02T18:17:53.342Z
Learning: In the Saint Vincent and the Grenadines holidays implementation, New Year's Day is added without observed rules using `_add_new_years_day()` and should not include observed rule testing in its test method. Only holidays explicitly wrapped with `_add_observed()` have observed behavior.
Applied to files:
holidays/financial/_sifma.py
🧬 Code graph analysis (1)
holidays/financial/_sifma.py (5)
holidays/groups/christian.py (3)
_add_good_friday(344-353)_add_christmas_day(224-232)_add_holy_thursday(364-373)holidays/groups/international.py (3)
InternationalHolidays(20-236)_add_new_years_day(142-150)_add_remembrance_day(182-190)holidays/observed_holiday_base.py (4)
ObservedHolidayBase(102-256)ObservedRule(20-24)_move_holiday(199-211)_get_observed_date(130-142)holidays/financial/us_new_york.py (2)
_populate_public_holidays(48-56)_populate_half_day_holidays(58-63)holidays/holiday_base.py (1)
_add_holiday(797-809)
🪛 Ruff (0.14.1)
holidays/financial/_sifma.py
48-48: Missing return type annotation for special method __init__
Add return type annotation: None
(ANN204)
48-48: Missing type annotation for *args
(ANN002)
48-48: Missing type annotation for **kwargs
(ANN003)
54-54: Missing return type annotation for private function _populate_public_holidays
Add return type annotation: None
(ANN202)
111-111: Missing return type annotation for private function _populate_half_day_holidays
Add return type annotation: None
(ANN202)
🔇 Additional comments (4)
holidays/financial/_sifma.py (4)
23-23: Early‑close rule mapping looks correct.Deltas cover Fri close for Sun/Mon holidays and Thu for Sat, with −1 for Tue–Fri. This matches SIFMA behavior for 2021–2023 edge cases.
62-80: Uniform Monday Holiday Act transitions are implemented correctly.Washington’s (pre‑1971 Feb 22 vs 3rd Mon), Memorial (May 30 vs last Mon), Columbus (Oct 12 vs 2nd Mon), and Veterans (1971–1977 4th Mon of Oct) are handled with year guards.
Also applies to: 91-104
124-133: Pre‑1971 Memorial Day early close logic is sound.Computing from May 30 with SIFMA_EARLY_CLOSE cleanly yields Fri/Thu adjustments without relying on PUBLIC dates.
If you want an extra sanity check, verify 1969–1971 deltas align with SIFMA tables for Sunday/Monday/Saturday cases.
135-139: Independence/Christmas early closes correctly derive from holiday dates via SIFMA_EARLY_CLOSE.This keeps HALF_DAY independent from PUBLIC and handles Sun/Mon cases (e.g., 2021–2023) as expected.
Also applies to: 144-148
KJhellico
reviewed
Oct 27, 2025
Co-authored-by: ~Jhellico <KJhellico@users.noreply.github.com> Signed-off-by: Ryley Mao <rymao_@outlook.com>
Co-authored-by: ~Jhellico <KJhellico@users.noreply.github.com> Signed-off-by: Ryley Mao <rymao_@outlook.com>
Co-authored-by: ~Jhellico <KJhellico@users.noreply.github.com> Signed-off-by: Ryley Mao <rymao_@outlook.com>
Co-authored-by: ~Jhellico <KJhellico@users.noreply.github.com> Signed-off-by: Ryley Mao <rymao_@outlook.com>
Co-authored-by: ~Jhellico <KJhellico@users.noreply.github.com> Signed-off-by: Ryley Mao <rymao_@outlook.com>
Co-authored-by: ~Jhellico <KJhellico@users.noreply.github.com> Signed-off-by: Ryley Mao <rymao_@outlook.com>
Co-authored-by: ~Jhellico <KJhellico@users.noreply.github.com> Signed-off-by: Ryley Mao <rymao_@outlook.com>
Co-authored-by: ~Jhellico <KJhellico@users.noreply.github.com> Signed-off-by: Ryley Mao <rymao_@outlook.com>
Co-authored-by: ~Jhellico <KJhellico@users.noreply.github.com> Signed-off-by: Ryley Mao <rymao_@outlook.com>
Co-authored-by: ~Jhellico <KJhellico@users.noreply.github.com> Signed-off-by: Ryley Mao <rymao_@outlook.com>
Co-authored-by: ~Jhellico <KJhellico@users.noreply.github.com> Signed-off-by: Ryley Mao <rymao_@outlook.com>
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (1)
holidays/financial/_sifma.py(1 hunks)
🧰 Additional context used
🧠 Learnings (31)
📚 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:
holidays/financial/_sifma.py
📚 Learning: 2025-06-14T11:05:21.250Z
Learnt from: PPsyrius
PR: vacanza/holidays#2609
File: holidays/countries/nauru.py:48-50
Timestamp: 2025-06-14T11:05:21.250Z
Learning: In the holidays library, newer implementations use `start_year` to indicate the earliest year with complete holiday data coverage, not necessarily the first year a holiday existed. If a holiday system starts partway through a year (like Nauru's Public Holidays Act starting Jan 31, 1968), the start_year should be set to the following year (1969) to ensure users get full annual holiday coverage.
Applied to files:
holidays/financial/_sifma.py
📚 Learning: 2025-06-14T11:04:31.180Z
Learnt from: PPsyrius
PR: vacanza/holidays#2609
File: holidays/countries/nauru.py:57-60
Timestamp: 2025-06-14T11:04:31.180Z
Learning: In the holidays library, the base `HolidayBase._populate()` method already includes a guard clause that prevents holiday population methods like `_populate_public_holidays()` from being called when the year is before `start_year` or after `end_year`. Therefore, individual country implementations do not need to add their own guard clauses for years before independence or other start dates.
Applied to files:
holidays/financial/_sifma.py
📚 Learning: 2025-08-26T14:43:53.605Z
Learnt from: KJhellico
PR: vacanza/holidays#2851
File: docs/holiday_categories.md:272-282
Timestamp: 2025-08-26T14:43:53.605Z
Learning: In the holidays library documentation, it's strongly advisable to recommend the use of constants from holidays.constants (e.g., PUBLIC, CATHOLIC) instead of direct string values when specifying holiday categories, as constants provide better type safety, IDE support, and prevent typos.
Applied to files:
holidays/financial/_sifma.py
📚 Learning: 2025-05-04T10:29:46.780Z
Learnt from: KJhellico
PR: vacanza/holidays#2525
File: holidays/countries/togo.py:0-0
Timestamp: 2025-05-04T10:29:46.780Z
Learning: When a country class in the holidays library uses additional categories beyond PUBLIC, the `supported_categories` tuple should contain all categories, including PUBLIC. Only when PUBLIC is the only category being used should it be omitted from `supported_categories`.
Applied to files:
holidays/financial/_sifma.py
📚 Learning: 2025-03-04T11:41:56.389Z
Learnt from: PPsyrius
PR: vacanza/holidays#2323
File: holidays/countries/macau.py:284-366
Timestamp: 2025-03-04T11:41:56.389Z
Learning: For Macau holidays implementation, maintaining separate methods for each holiday category (PUBLIC, MANDATORY, GOVERNMENT) is preferred because these categories are based on different sets of laws and have distinct historical evolution.
Applied to files:
holidays/financial/_sifma.py
📚 Learning: 2025-03-05T02:35:03.298Z
Learnt from: PPsyrius
PR: vacanza/holidays#2323
File: holidays/countries/macau.py:278-377
Timestamp: 2025-03-05T02:35:03.298Z
Learning: For Macau holiday implementations, it's preferable to maintain separate methods for different holiday categories (MANDATORY, GOVERNMENT, PUBLIC) as they are based on different sets of laws, making the code easier to maintain despite having multiple year-based conditionals.
Applied to files:
holidays/financial/_sifma.py
📚 Learning: 2025-03-04T11:32:45.095Z
Learnt from: PPsyrius
PR: vacanza/holidays#2323
File: holidays/countries/macau.py:407-461
Timestamp: 2025-03-04T11:32:45.095Z
Learning: In the holidays library, the standard approach for organizing static holidays is to use separate dictionaries for different categories (`government`, `mandatory`, and `public`), which are utilized by syntactic sugar methods.
Applied to files:
holidays/financial/_sifma.py
📚 Learning: 2025-06-18T10:58:28.058Z
Learnt from: PPsyrius
PR: vacanza/holidays#2642
File: holidays/countries/france.py:184-206
Timestamp: 2025-06-18T10:58:28.058Z
Learning: In the holidays library France implementation, creating shared private methods between subdivision-specific holiday population methods (like _populate_subdiv_57_public_holidays and _populate_subdiv_6ae_public_holidays) is not supported by the current framework architecture, so duplicate code between functionally identical subdivision methods should be left as-is.
Applied to files:
holidays/financial/_sifma.py
📚 Learning: 2025-08-28T02:42:52.755Z
Learnt from: PPsyrius
PR: vacanza/holidays#2863
File: tests/countries/test_georgia.py:31-36
Timestamp: 2025-08-28T02:42:52.755Z
Learning: In the holidays framework, when no categories parameter is specified in a country class instantiation (e.g., `Georgia(years=2025)`), the PUBLIC category is used by default. There's no need to explicitly specify `categories=PUBLIC` or import the PUBLIC constant for such test cases.
Applied to files:
holidays/financial/_sifma.py
📚 Learning: 2025-04-08T14:46:10.656Z
Learnt from: KJhellico
PR: vacanza/holidays#2437
File: holidays/countries/bhutan.py:27-30
Timestamp: 2025-04-08T14:46:10.656Z
Learning: For country classes in the holidays library, there's no need to explicitly specify `supported_categories = (PUBLIC,)` when PUBLIC is the only category being used, as it's already the default category inherited from HolidayBase.
Applied to files:
holidays/financial/_sifma.py
📚 Learning: 2025-03-04T11:32:45.095Z
Learnt from: PPsyrius
PR: vacanza/holidays#2323
File: holidays/countries/macau.py:407-461
Timestamp: 2025-03-04T11:32:45.095Z
Learning: In the holidays library, the standard approach for organizing static holidays is to use separate dictionaries for different categories (like `special_government_holidays`, `special_mandatory_holidays`, and `special_public_holidays`), which are utilized by syntactic sugar methods to pick up the appropriate holidays based on the selected category.
Applied to files:
holidays/financial/_sifma.py
📚 Learning: 2025-06-18T10:21:01.376Z
Learnt from: PPsyrius
PR: vacanza/holidays#2642
File: holidays/countries/france.py:300-319
Timestamp: 2025-06-18T10:21:01.376Z
Learning: In the France holidays implementation, legislative years for holiday changes should be hard-coded rather than extracted into constants, as this maintains consistency with the existing codebase pattern and provides historical accuracy for specific legislative acts.
Applied to files:
holidays/financial/_sifma.py
📚 Learning: 2025-07-14T20:23:48.198Z
Learnt from: KJhellico
PR: vacanza/holidays#2654
File: holidays/countries/cabo_verde.py:133-141
Timestamp: 2025-07-14T20:23:48.198Z
Learning: The holidays library provides helper methods `_add_holiday_2nd_sun_of_may()` and `_add_holiday_3rd_sun_of_jun()` for adding holidays on the 2nd Sunday of May and 3rd Sunday of June respectively. These methods are used across multiple country implementations including Latvia, Finland, Belarus, Malaysia, Madagascar, and Cape Verde.
Applied to files:
holidays/financial/_sifma.py
📚 Learning: 2025-05-13T13:23:11.375Z
Learnt from: KJhellico
PR: vacanza/holidays#2483
File: holidays/countries/turks_and_caicos_islands.py:117-118
Timestamp: 2025-05-13T13:23:11.375Z
Learning: The holidays library uses `_add_christmas_day_two` method to add Boxing Day holiday, not `_add_boxing_day`.
Applied to files:
holidays/financial/_sifma.py
📚 Learning: 2025-06-14T20:43:15.370Z
Learnt from: KJhellico
PR: vacanza/holidays#2631
File: holidays/countries/sint_maarten.py:94-95
Timestamp: 2025-06-14T20:43:15.370Z
Learning: The `_add_*` helper methods in the holidays library (such as `_add_christmas_day_two()`, `_add_labor_day()`, etc.) don't have default holiday names, so the name parameter should always be explicitly specified when calling these methods.
Applied to files:
holidays/financial/_sifma.py
📚 Learning: 2025-04-03T16:58:27.175Z
Learnt from: Wasif-Shahzad
PR: vacanza/holidays#2409
File: holidays/countries/qatar.py:27-46
Timestamp: 2025-04-03T16:58:27.175Z
Learning: In the holidays library, method names like `_add_holiday_2nd_tue_of_feb()` and `_add_holiday_1st_sun_of_mar()` use calendar constants like FEB, TUE, MAR, and SUN internally through parent class implementations even when these constants don't appear directly in the file.
Applied to files:
holidays/financial/_sifma.py
📚 Learning: 2025-08-19T21:00:47.849Z
Learnt from: KJhellico
PR: vacanza/holidays#2831
File: holidays/countries/south_sudan.py:84-88
Timestamp: 2025-08-19T21:00:47.849Z
Learning: In the holidays library, Islamic holidays use dedicated methods for additional days (like `_add_eid_al_fitr_day_two`, `_add_eid_al_adha_day_two`) rather than parameters. The methods don't accept a `days` parameter - each day has its own specific method.
Applied to files:
holidays/financial/_sifma.py
📚 Learning: 2025-07-09T21:16:35.145Z
Learnt from: KJhellico
PR: vacanza/holidays#2623
File: tests/countries/test_christmas_island.py:136-146
Timestamp: 2025-07-09T21:16:35.145Z
Learning: In Christmas Island's holiday implementation, the test_christmas_day method cannot use assertNoNonObservedHoliday because in some years observed Christmas Day overlaps with Boxing Day when both holidays are moved due to weekend conflicts, causing the standard non-observed holiday check to fail inappropriately.
Applied to files:
holidays/financial/_sifma.py
📚 Learning: 2025-06-16T12:28:31.641Z
Learnt from: PPsyrius
PR: vacanza/holidays#2632
File: holidays/countries/solomon_islands.py:95-98
Timestamp: 2025-06-16T12:28:31.641Z
Learning: Library-wide holiday patterns and their optimizations should be handled at the base class level (like InternationalHolidays) rather than documenting workarounds in individual country modules. This maintains separation of concerns and avoids documentation duplication.
Applied to files:
holidays/financial/_sifma.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:
holidays/financial/_sifma.py
📚 Learning: 2025-09-12T21:37:10.710Z
Learnt from: KJhellico
PR: vacanza/holidays#2854
File: tests/countries/test_sudan.py:29-31
Timestamp: 2025-09-12T21:37:10.710Z
Learning: For countries in the holidays library that only use the default PUBLIC category (like Sudan), there's no need to specify `categories=Sudan.supported_categories` in test_no_holidays methods, as it would be equivalent to the default behavior.
Applied to files:
holidays/financial/_sifma.py
📚 Learning: 2025-09-14T16:19:23.651Z
Learnt from: PPsyrius
PR: vacanza/holidays#2881
File: tests/countries/test_antigua_and_barbuda.py:27-29
Timestamp: 2025-09-14T16:19:23.651Z
Learning: For Antigua and Barbuda in the holidays library, only the default PUBLIC category is used, so there's no need to specify `categories=AntiguaAndBarbuda.supported_categories` in test_no_holidays methods, as it would be equivalent to the default behavior and AntiguaAndBarbuda doesn't define supported_categories.
Applied to files:
holidays/financial/_sifma.py
📚 Learning: 2025-09-14T16:02:15.480Z
Learnt from: PPsyrius
PR: vacanza/holidays#2881
File: tests/countries/test_bahamas.py:27-29
Timestamp: 2025-09-14T16:02:15.480Z
Learning: For Bahamas in the holidays library, only the default PUBLIC category is used, so there's no need to specify `categories=Bahamas.supported_categories` in test_no_holidays methods, as it would be equivalent to the default behavior.
Applied to files:
holidays/financial/_sifma.py
📚 Learning: 2025-08-03T13:48:11.910Z
Learnt from: KJhellico
PR: vacanza/holidays#2777
File: holidays/countries/gambia.py:120-122
Timestamp: 2025-08-03T13:48:11.910Z
Learning: When reviewing holiday implementations in the holidays library, defer to the maintainers' choice of start years for specific holiday policies, as they likely have access to more reliable primary sources and official documentation than what can be found through web searches.
Applied to files:
holidays/financial/_sifma.py
📚 Learning: 2025-10-21T09:59:18.714Z
Learnt from: KJhellico
PR: vacanza/holidays#3008
File: tests/financial/test_us_new_york.py:22-26
Timestamp: 2025-10-21T09:59:18.714Z
Learning: In the holidays (vacanza) project, do not suggest adding return type annotations. The project will address return type annotations comprehensively in the future rather than incrementally.
Applied to files:
holidays/financial/_sifma.py
📚 Learning: 2025-04-23T09:22:41.753Z
Learnt from: PPsyrius
PR: vacanza/holidays#2489
File: holidays/countries/sao_tome_and_principe.py:86-88
Timestamp: 2025-04-23T09:22:41.753Z
Learning: For holiday definitions in the holidays package, keep comments simple with just the holiday name (e.g., "# Independence Day.") rather than including dates or historical context, as the function names already encode the date information.
Applied to files:
holidays/financial/_sifma.py
📚 Learning: 2025-08-19T21:22:13.125Z
Learnt from: KJhellico
PR: vacanza/holidays#2831
File: holidays/countries/south_sudan.py:84-88
Timestamp: 2025-08-19T21:22:13.125Z
Learning: In the holidays library, message comments directly above holiday addition methods must match the holiday name exactly without any additions, modifications, or explanatory text. For example, if the holiday name is "Eid al-Fitr Holiday", the comment should be "# Eid al-Fitr Holiday." with no extra context.
Applied to files:
holidays/financial/_sifma.py
📚 Learning: 2025-05-10T04:02:13.815Z
Learnt from: PPsyrius
PR: vacanza/holidays#2537
File: holidays/countries/finland.py:249-253
Timestamp: 2025-05-10T04:02:13.815Z
Learning: Holiday name comments directly above tr() function calls in the holidays package should only contain the holiday name itself (e.g., "# Independence Day.") without any additional context, dates, or historical information.
Applied to files:
holidays/financial/_sifma.py
📚 Learning: 2025-05-10T04:32:15.760Z
Learnt from: PPsyrius
PR: vacanza/holidays#2537
File: holidays/countries/finland.py:0-0
Timestamp: 2025-05-10T04:32:15.760Z
Learning: In the holidays package, detailed historical context and additional information should be added as comments at the method level or above conditional blocks, while comments directly above tr() function calls should only contain the holiday name itself (e.g., "# Independence Day.").
Applied to files:
holidays/financial/_sifma.py
📚 Learning: 2025-07-02T18:17:53.342Z
Learnt from: KJhellico
PR: vacanza/holidays#2608
File: tests/countries/test_saint_vincent_and_the_grenadines.py:162-178
Timestamp: 2025-07-02T18:17:53.342Z
Learning: In the Saint Vincent and the Grenadines holidays implementation, New Year's Day is added without observed rules using `_add_new_years_day()` and should not include observed rule testing in its test method. Only holidays explicitly wrapped with `_add_observed()` have observed behavior.
Applied to files:
holidays/financial/_sifma.py
🧬 Code graph analysis (1)
holidays/financial/_sifma.py (4)
holidays/groups/christian.py (3)
_add_good_friday(344-353)_add_christmas_day(224-232)_add_holy_thursday(364-373)holidays/groups/international.py (4)
InternationalHolidays(20-236)_add_new_years_day(142-150)_add_columbus_day(80-88)_add_remembrance_day(182-190)holidays/observed_holiday_base.py (4)
ObservedHolidayBase(102-256)ObservedRule(20-24)_move_holiday(199-211)_get_observed_date(130-142)holidays/holiday_base.py (1)
_add_holiday(797-809)
🪛 Ruff (0.14.1)
holidays/financial/_sifma.py
50-50: Missing return type annotation for special method __init__
Add return type annotation: None
(ANN204)
50-50: Missing type annotation for *args
(ANN002)
50-50: Missing type annotation for **kwargs
(ANN003)
56-56: Missing return type annotation for private function _populate_public_holidays
Add return type annotation: None
(ANN202)
113-113: Missing return type annotation for private function _populate_half_day_holidays
Add return type annotation: None
(ANN202)
148-148: Undefined name DEC
(F821)
🔇 Additional comments (3)
holidays/financial/_sifma.py (3)
1-26: Solid foundation—imports and SIFMA_EARLY_CLOSE rule look correct.The SIFMA_EARLY_CLOSE rule correctly handles the day-before logic: Monday holidays → Friday (-3), Sunday holidays → Friday (-2), Tue-Fri holidays → prior day (-1), Saturday holidays → Thursday (-2). This matches the SIFMA recommendations discussed in earlier reviews.
28-54: Class structure and initialization are well-designed.The multiple inheritance order is correct, parent classes are properly initialized, and the default observed rule (SAT_TO_PREV_FRI + SUN_TO_NEXT_MON) aligns with standard U.S. federal holiday observance. The docstring provides clear context and references.
56-113: PUBLIC holidays implementation is thorough and historically accurate.The year-based transitions are correctly implemented:
- Washington's Birthday, Memorial Day, Columbus Day handle the Uniform Monday Holiday Act (pre/post-1971)
- Veterans Day correctly uses 4th Monday of October for 1971-1977, then returns to November 11
- MLK Day starts in 1998, Juneteenth in 2021
All observance rules are properly applied. Well done.
KJhellico
reviewed
Oct 28, 2025
| def _populate_public_holidays(self): | ||
| # Call parent to get all SIFMA holidays | ||
| super()._populate_public_holidays() | ||
|
|
There was a problem hiding this comment.
It is more logical to take a less complex entity as a basis and add elements to it than to take a more complex one and then remove elements. In general, if you do not plan to add any other markets based on SIFMA, it would be optimal to implement the USNY class and, based on it, USGS (adding Good Friday), without a separate SIFMA class.
There was a problem hiding this comment.
I see what you are saying and agree with you, building from USNY (simpler) up to USGS (more complex) is more logical than the current approach.
I am wondering, given that all previous review comments have been addressed and tests are passing, would you be open to merging this PR as is and addressing the refactoring in a follow up issue? I'm happy to take on that refactoring work and be assigned to it. If you'd prefer the refactoring done now before merge, I can do that too. Just let me know your preference.
There was a problem hiding this comment.
I think it's better to do everything properly from the start.
BTW, about tests: I don't review the tests in detail until the code for the holidays themselves is fully agreed upon, but looking at them now, I see that they don't quite meet our latest standards for tests.
Co-authored-by: ~Jhellico <KJhellico@users.noreply.github.com> Signed-off-by: Ryley Mao <rymao_@outlook.com>
Co-authored-by: ~Jhellico <KJhellico@users.noreply.github.com> Signed-off-by: Ryley Mao <rymao_@outlook.com>
Co-authored-by: ~Jhellico <KJhellico@users.noreply.github.com> Signed-off-by: Ryley Mao <rymao_@outlook.com>
- Move all SIFMA holiday logic from _sifma.py to USNewYork class - USNewYork is now the simpler base (no Good Friday) - USGovernmentSecurities inherits from USNewYork and adds Good Friday - Delete _sifma.py as it's no longer needed - This follows the principle of building from simpler to more complex Addresses KJhellico's architectural feedback
|
KJhellico
reviewed
Oct 30, 2025
| - USGS observes Good Friday as a full closure (USNY does not) | ||
| References: | ||
| * <https://www.sifma.org/resources/general/holiday-schedule/> |
There was a problem hiding this comment.
Suggested change
| * <https://www.sifma.org/resources/general/holiday-schedule/> |
Comment on lines
+43
to
+59
| def _populate_public_holidays(self): | ||
| # Get all USNY holidays first | ||
| super()._populate_public_holidays() | ||
|
|
||
| # Note: Some SIFMA markets may treat this as an early close instead of full closure. | ||
|
|
||
| # Good Friday. | ||
| self._add_good_friday("Good Friday") | ||
|
|
||
| def _populate_half_day_holidays(self): | ||
| # Get all USNY early closes first | ||
| super()._populate_half_day_holidays() | ||
|
|
||
| # Add Good Friday early close. | ||
| # Day before Good Friday (Maundy Thursday). | ||
| begin_time_label = "Markets close at 2:00 PM ET (%s)" | ||
| self._add_holy_thursday(begin_time_label % "Good Friday") |
There was a problem hiding this comment.
Suggested change
| def _populate_public_holidays(self): | |
| # Get all USNY holidays first | |
| super()._populate_public_holidays() | |
| # Note: Some SIFMA markets may treat this as an early close instead of full closure. | |
| # Good Friday. | |
| self._add_good_friday("Good Friday") | |
| def _populate_half_day_holidays(self): | |
| # Get all USNY early closes first | |
| super()._populate_half_day_holidays() | |
| # Add Good Friday early close. | |
| # Day before Good Friday (Maundy Thursday). | |
| begin_time_label = "Markets close at 2:00 PM ET (%s)" | |
| self._add_holy_thursday(begin_time_label % "Good Friday") | |
| def _populate_public_holidays(self): | |
| super()._populate_public_holidays() | |
| # Good Friday. | |
| self._add_good_friday("Good Friday") | |
| def _populate_half_day_holidays(self): | |
| super()._populate_half_day_holidays() | |
| begin_time_label = "Markets close at 2:00 PM ET (%s)" | |
| # Day before Good Friday. | |
| self._add_holy_thursday(begin_time_label % "Good Friday") |
Comment on lines
+66
to
+67
| class USBondMarket(USGovernmentSecurities): | ||
| pass |
There was a problem hiding this comment.
How standard is USBondMarket name?
Comment on lines
+31
to
+39
| The USNY calendar represents general "New York business days" used in financial contracts | ||
| (ISDA derivatives, OTC instruments). While it follows SIFMA (Securities Industry and | ||
| Financial Markets Association) recommendations for most holidays, it differs from both | ||
| USGS (US Government Securities) and NYSE calendars. | ||
| Key differences: | ||
| - vs USGS: USNY does NOT observe Good Friday (USGS does) | ||
| - vs NYSE: USNY DOES observe Columbus Day and Veterans Day (NYSE does not in modern times) | ||
| - vs NYSE: USNY does NOT include special one-off closures (NYSE does) |
There was a problem hiding this comment.
As I can see, USNY is actually New York Banking Holidays, and don't follow SIFMA. If you know of sources that confirm otherwise, please cite them.
KJhellico
reviewed
Oct 30, 2025
Comment on lines
+219
to
+221
| 1 | ||
| for path in Path("holidays/financial").glob("*.py") | ||
| if path.stem != "__init__" and not path.stem.startswith("_") |
There was a problem hiding this comment.
Suggested change
| 1 | |
| for path in Path("holidays/financial").glob("*.py") | |
| if path.stem != "__init__" and not path.stem.startswith("_") | |
| 1 for path in Path("holidays/financial").glob("*.py") if path.stem != "__init__" |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Implements support for US Government Securities (USGS) and US New York (USNY) financial market holiday calendars following SIFMA recommendations.
Resolves #852.
Changes
USGovernmentSecuritiesclass with full bond market holidays including Good Friday, Columbus Day, and Veterans DayUSNewYorkclass with standard New York financial market holidaysTesting
All tests pass:
python3 -m pytest tests/financial/test_us_government_securities.py tests/financial/test_us_new_york.py -v # 44 passed in 0.08s