Skip to content
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

Merged
merged 12 commits into from
Jun 28, 2023

Conversation

KJhellico
Copy link
Collaborator

@KJhellico KJhellico commented Jun 20, 2023

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:

>>> from holidays import country_holidays
>>> from holidays.categories import BANK, PUBLIC
>>> at = country_holidays("AT")  # "common" holidays as default
>>> "2023-12-31" in at
False
>>> at_full = country_holidays("AT", categories=(BANK, PUBLIC))
>>> "2023-12-31" in at_full
True
>>> at_full["2023-12-31"]
Silvester

Type of change

  • New country/market holidays support (thank you!)
  • Supported country/market holidays update (calendar discrepancy fix, localization)
  • Existing code/documentation/test/process quality improvement (best practice, cleanup, refactoring, optimization)
  • Dependency upgrade (version update)
  • Bugfix (non-breaking change which fixes an issue)
  • Breaking change (a code change causing existing functionality to break)
  • New feature (new python-holidays functionality in general)

Checklist

  • I've followed the contributing guidelines
  • This PR is filed against beta branch of the repository
  • This PR doesn't contain any merge conflicts and has clean commit history
  • The code style looks good: make pre-commit
  • All tests pass locally: make test, make tox (we strongly encourage adding tests to your code)
  • The related documentation has been added/updated (check off the box for free if no updates is required)

@KJhellico KJhellico requested a review from arkid15r June 20, 2023 13:07
@coveralls
Copy link

coveralls commented Jun 20, 2023

Coverage Status

coverage: 100.0%. remained the same when pulling ce955be on KJhellico:feat-holiday-categories into ddf111e on dr-prodigy:beta.

@PPsyrius
Copy link
Collaborator

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.

@KJhellico
Copy link
Collaborator Author

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?

Yes, I think such holidays can be added as "half-day" category (or something like). Maybe with (or without) some additional label...

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.

These are exactly the cases I meant!

Copy link
Collaborator

@arkid15r arkid15r left a 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.

holidays/countries/austria.py Outdated Show resolved Hide resolved
holidays/holiday_base.py Show resolved Hide resolved
holidays/utils.py Outdated Show resolved Hide resolved
holidays/holiday_base.py Outdated Show resolved Hide resolved
holidays/holiday_base.py Outdated Show resolved Hide resolved
holidays/holiday_base.py Show resolved Hide resolved
Copy link
Collaborator

@arkid15r arkid15r left a 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:

holidays/countries/austria.py Outdated Show resolved Hide resolved
@KJhellico
Copy link
Collaborator Author

KJhellico commented Jun 23, 2023

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.

It's worth to think about it. 👍

@KJhellico
Copy link
Collaborator Author

KJhellico commented Jun 23, 2023

But then we need to switch all countries at once to support categories. In order to have supported_categories = ("public",) everywhere.

@KJhellico KJhellico force-pushed the feat-holiday-categories branch from 4c98230 to 4823187 Compare June 23, 2023 10:39
Copy link
Collaborator

@arkid15r arkid15r left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Some suggestions:

holidays/categories.py Outdated Show resolved Hide resolved
holidays/categories.py Outdated Show resolved Hide resolved
holidays/categories.py Outdated Show resolved Hide resolved
holidays/categories.py Outdated Show resolved Hide resolved
holidays/categories.py Outdated Show resolved Hide resolved
holidays/holiday_base.py Outdated Show resolved Hide resolved
holidays/holiday_base.py Outdated Show resolved Hide resolved
tests/countries/test_austria.py Outdated Show resolved Hide resolved
holidays/categories.py Outdated Show resolved Hide resolved
holidays/holiday_base.py Show resolved Hide resolved
holidays/holiday_base.py Outdated Show resolved Hide resolved
@KJhellico KJhellico requested a review from arkid15r June 28, 2023 11:10
Copy link
Collaborator

@arkid15r arkid15r left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

holidays/utils.py Outdated Show resolved Hide resolved
@arkid15r arkid15r changed the title Introduce holiday categories support [PoC] Introduce holiday categories support Jun 28, 2023
@arkid15r arkid15r added the ready for beta Ready to merge on beta branch label Jun 28, 2023
@arkid15r arkid15r merged commit 7093c3b into vacanza:beta Jun 28, 2023
@arkid15r arkid15r removed the ready for beta Ready to merge on beta branch label Jun 28, 2023
@PPsyrius
Copy link
Collaborator

@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),
        )

@KJhellico KJhellico deleted the feat-holiday-categories branch June 29, 2023 10:23
@KJhellico
Copy link
Collaborator Author

KJhellico commented Jun 29, 2023

@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:

Did you replace _populate() with _populate_public_holidays() for Thailand? (and year with self._year inside it)

@KJhellico
Copy link
Collaborator Author

I see, problem is special holidays, they are always populated :)

@PPsyrius
Copy link
Collaborator

I see, problem is special holidays, they are always populated :)

Oh, that explains it

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants