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

Update Thailand holidays: add holiday categories #1346

Merged
merged 34 commits into from Sep 12, 2023

Conversation

PPsyrius
Copy link
Contributor

@PPsyrius PPsyrius commented Jun 29, 2023

Proposed change

Following the changes in #1320, Thailand holidays now support holiday categories.

Change List from 0.27 Release (More incoming, still WIP)

ARMED_FORCES:
+ Royal Thai Armed Forces Day

BANK:
+ Additional Closing Day for Bank for Agriculture and Agricultural Cooperatives
+ Mid-Year Closing Day

GOVERNMENT:
+ Royal Ploughing Ceremony (Moved from PUBLIC since this only apply to government employees)

PUBLIC (Old Default Category):
+ National Children's Day (Turns out these are National Public Holidays rather than just Observed National Days)
- Royal Ploughing Ceremony (see above)

SCHOOL
+ Teacher's Day

WORKDAY
+ Thai Veterans Day
+ National Science Day
+ National Artist Day
+ International Women's Day
+ National Forest Conservation Day
+ HM King Ramkamhaeng Memorial Day
+ National Aviation Day
+ Thai National Flag Day
+ Loy Krathong

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)

@coveralls
Copy link

coveralls commented Jun 29, 2023

Coverage Status

coverage: 100.0%. remained the same when pulling 9356730 on PPsyrius:thailand_categories into 85b377d on dr-prodigy:beta.

Copy link
Collaborator

@KJhellico KJhellico left a comment

Choose a reason for hiding this comment

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

Great! 🚀 I'm happy that my idea with categories was useful. Now I have extended it to special holidays as well.

holidays/countries/thailand.py Outdated Show resolved Hide resolved
holidays/countries/thailand.py Outdated Show resolved Hide resolved
tests/countries/test_thailand.py Outdated Show resolved Hide resolved
PPsyrius and others added 4 commits June 29, 2023 22:41
Co-Authored-By: ~Jhellico <KJhellico@users.noreply.github.com>
Co-Authored-By: ~Jhellico <KJhellico@users.noreply.github.com>
PPsyrius and others added 2 commits July 4, 2023 11:18
@arkid15r arkid15r changed the title Thailand Holiday Categories Update Thailand holidays: add holiday categories Jul 5, 2023
@arkid15r
Copy link
Collaborator

arkid15r commented Jul 5, 2023

Not sure if it's still WIP. Please update the description if it's ready for review.

@PPsyrius
Copy link
Contributor Author

PPsyrius commented Jul 6, 2023

@arkid15r I'm afraid this is still very WIP as I've yet got the time to implement their National Date of Importance List for workday category in full

@arkid15r arkid15r marked this pull request as draft July 10, 2023 19:28
@PPsyrius
Copy link
Contributor Author

PPsyrius commented Aug 1, 2023

tests/common.py:237: in _assertHolidays
    self.assertEqual(
E   AssertionError: 2287 != 28 : {('2021-02-12', 'วันหยุดพิเศษ (เพิ่มเติม)'), ('2045-01-14', 'วันเด็กแห่งชาติ'), ('1975-05-05', 'วันฉัตรมงคล'), ('1990-04-13', 'วันสงกรานต์'), ('2000-04-13', 'วันสงกรานต์'), ...

Maybe I should just restart this as a new PR at this rate

@arkid15r
Copy link
Collaborator

arkid15r commented Aug 1, 2023

Maybe I should just restart this as a new PR at this rate

Hey @PPsyrius, it's totally fine to close it in favor of a new one or continue your work within this PR. Whatever works best for you.

Thank you 👍

Copy link
Collaborator

@KJhellico KJhellico left a comment

Choose a reason for hiding this comment

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

And test is pass.

tests/countries/test_thailand.py Outdated Show resolved Hide resolved
tests/countries/test_thailand.py Outdated Show resolved Hide resolved
tests/countries/test_thailand.py Outdated Show resolved Hide resolved
PPsyrius and others added 6 commits August 3, 2023 09:48
Co-authored-by: ~Jhellico <KJhellico@users.noreply.github.com>
Co-authored-by: ~Jhellico <KJhellico@users.noreply.github.com>
Co-authored-by: ~Jhellico <KJhellico@users.noreply.github.com>
@coveralls
Copy link

coveralls commented Aug 3, 2023

Pull Request Test Coverage Report for Build 6128863971

  • 74 of 74 (100.0%) changed or added relevant lines in 2 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage remained the same at 100.0%

Totals Coverage Status
Change from base Build 6125338670: 0.0%
Covered Lines: 9160
Relevant Lines: 9160

💛 - Coveralls

Copy link
Collaborator

@KJhellico KJhellico left a comment

Choose a reason for hiding this comment

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

LGTM. Are you planning to add any more holidays?

holidays/locale/en_US/LC_MESSAGES/TH.po Outdated Show resolved Hide resolved
holidays/locale/th/LC_MESSAGES/TH.po Outdated Show resolved Hide resolved
PPsyrius and others added 2 commits September 8, 2023 10:39
Co-Authored-By: ~Jhellico <KJhellico@users.noreply.github.com>
@PPsyrius
Copy link
Contributor Author

PPsyrius commented Sep 8, 2023

LGTM. Are you planning to add any more holidays?

On additional workday ("Date of Significance" according to gov.) - IMO, we're still missing 30+ of them, but the rest are so miscellaneous we may as well merge this PR as-is then put the rest in later in the future. Any suggestion on how to proceed with this would be appreciated. @arkid15r @KJhellico

@KJhellico
Copy link
Collaborator

I think, it's time to merge this PR.

@arkid15r
Copy link
Collaborator

arkid15r commented Sep 8, 2023

Any suggestion on how to proceed with this would be appreciated. @arkid15r @KJhellico

Well, it's up to you. I don't see any reasons to not make it ready for review if you feel so.
Great work here on this complex holiday calendar 👍
Thank you, @PPsyrius!

@PPsyrius PPsyrius marked this pull request as ready for review September 9, 2023 04:19
arkid15r
arkid15r previously approved these changes Sep 12, 2023
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/countries/thailand.py Outdated Show resolved Hide resolved
@arkid15r
Copy link
Collaborator

This is another great update, I appreciate your work @PPsyrius.

This PR is in the merge queue and will be submitted automatically once tests are passed.

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.

It needed another approval though

@arkid15r arkid15r added this pull request to the merge queue Sep 12, 2023
Merged via the queue into vacanza:beta with commit 06bc39f Sep 12, 2023
22 checks passed
@KJhellico KJhellico mentioned this pull request Sep 18, 2023
@PPsyrius PPsyrius deleted the thailand_categories branch September 22, 2023 02:04
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.

None yet

4 participants