-
Notifications
You must be signed in to change notification settings - Fork 488
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add Saint Lucia holidays #2266
Add Saint Lucia holidays #2266
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## dev #2266 +/- ##
=========================================
Coverage 100.00% 100.00%
=========================================
Files 199 200 +1
Lines 12144 12177 +33
Branches 1731 1731
=========================================
+ Hits 12144 12177 +33 ☔ View full report in Codecov by Sentry. |
According to Bank Holidays Act
@Prateekshit73, you can apply observed holidays support for LC. The rules will be similar, for example, to Saint Kitts and Nevis. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks pretty good, just a few typoes here:
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Here are the remaining test case changes as previously requested by @KJhellico as well as a missing one for Emancipation Day
:
def test_emancipation_day(self):
name = "Emancipation Day"
self.assertHolidayName(name, (f"{year}-08-01" for year in range(1979, 2050)))
dt = (
"2004-08-02",
"2010-08-02",
"2021-08-02",
)
self.assertHolidayName(f"{name} (observed)", dt)
self.assertNoNonObservedHoliday(dt)
Aside from that, this PR looks more or less ready for merging to me 👍
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM 🇱🇨
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Another country! We're getting closer to 160 target.
Thank you @Prateekshit73 and @KJhellico @PPsyrius for reviewing
🇱🇨 LGTM
Proposed change
Added Saint Lucia holidays
Closes #1262
Type of change
holidays
functionality in general)Checklist
make check
, all checks and tests are green