-
Notifications
You must be signed in to change notification settings - Fork 470
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
Introduce holiday categories support #1320
Conversation
Good work IMO, although I got some questions here: Should half-day holidays i.e. #784 or Curacao's New Year's Eve holiday (see in-code comment in #1309), be included in this PR or is it considered an entirely-different feature altogether? For religious holidays another good example might be Argentina where Muslim and Jewish citizens got extra holidays on top of mainstream Christian ones, as well as an additional holidays that apply to citizens of Armenian origin only. |
Yes, I think such holidays can be added as "half-day" category (or something like). Maybe with (or without) some additional label...
These are exactly the cases I meant! |
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.
This PoC looks great!
Besides the naming suggestions below I'd also like you to address category support check, i.e. raise NotImplementedError
(duh) for cases when a requested category isn't supported.
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.
👍
Just one more idea for improvement:
It's worth to think about it. 👍 |
|
4c98230
to
4823187
Compare
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.
Some suggestions:
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
@KJhellico Hi, I'm working on extending the testcases right now for Thailand Holidays categories and yet it keep returning non-Bank/Government Holidays for my test results: def test_raeknakhwan(self):
name = "วันพืชมงคล"
self.assertHolidays(
Thailand(categories={GOVERNMENT}, years=range(1997, 2024)),
("1997-05-13", name),
("1998-05-13", name),
("2000-05-15", name),
("2001-05-16", name),
("2002-05-09", name),
("2003-05-08", name),
("2004-05-07", name),
("2005-05-11", name),
("2006-05-11", name),
("2007-05-10", name),
("2008-05-09", name),
("2009-05-11", name),
("2010-05-10", name),
("2011-05-13", name),
("2012-05-09", name),
("2013-05-13", name),
("2014-05-09", name),
("2015-05-13", name),
("2016-05-09", name),
("2017-05-12", name),
("2018-05-14", name),
("2019-05-09", name),
("2020-05-11", name),
("2021-05-13", name),
("2022-05-17", name),
("2023-05-11", name),
)
def test_bank_holiday(self):
a_name = "วันหยุดเพิ่มเติมสำหรับการปิดบัญชีประจำปีของธนาคารเพื่อการเกษตรและสหกรณ์การเกษตร"
m_name = "วันหยุดภาคครึ่งปีของสถาบันการเงินและสถาบันการเงินเฉพาะกิจ"
self.assertHolidays(
Thailand(categories={BANK}, years=range(2012, 2023)),
("2012-04-01", a_name),
("2012-07-01", m_name),
("2013-04-01", a_name),
("2013-07-01", m_name),
("2014-04-01", a_name),
("2014-07-01", m_name),
("2015-04-01", a_name),
("2015-07-01", m_name),
("2016-04-01", a_name),
("2016-07-01", m_name),
("2017-04-01", a_name),
("2017-07-01", m_name),
("2018-04-01", a_name),
("2018-07-01", m_name),
("2019-04-01", a_name),
("2020-04-01", a_name),
("2021-04-01", a_name),
) |
Did you replace |
I see, problem is special holidays, they are always populated :) |
Oh, that explains it |
Proposed change
Currently, Python Holidays supports only main country holidays ("state holidays", or "federal holidays", or "paid holidays" etc). But many countries have other categories of holidays: bank holidays, school holidays, additional (paid or non-paid) holidays, religious holidays (valid only for these religions followers). So this PR introduce initial holiday categories support.
Set of existing holidays matches "PUBLIC" category (that is populated by default, if
categories
value is not specified).For example:
Type of change
Checklist
beta
branch of the repositorymake pre-commit
make test
,make tox
(we strongly encourage adding tests to your code)