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

Add Tonga holidays #1534

Merged
merged 18 commits into from
Nov 2, 2023
Merged

Add Tonga holidays #1534

merged 18 commits into from
Nov 2, 2023

Conversation

PPsyrius
Copy link
Contributor

Proposed change

Add Tonga holidays (en_US, to localization).

Closes #1288.

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 update (version deprecation/upgrade)
  • 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
  • I've added references to all holidays information sources used in this PR
  • The code style looks good: make pre-commit command generates no changes
  • All tests pass locally: make test, make tox (we strongly encourage adding tests to your code)

@PPsyrius
Copy link
Contributor Author

PPsyrius commented Oct 24, 2023

This PR is marked as a draft should self._add_anzac_day(name) be added via #1531 - any help simplifying the holiday triggers would be greatly appreciated here. 👀

@coveralls
Copy link

coveralls commented Oct 24, 2023

Pull Request Test Coverage Report for Build 6710875690

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

Totals Coverage Status
Change from base Build 6678081414: 0.0%
Covered Lines: 10143
Relevant Lines: 10143

💛 - 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.

@PPsyrius , that's great! A new and exciting country! Please look at some refactoring suggestions.

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

I have some doubts about the exact meaning of Tonga observed holidays. Is it just an extra day off or is the actual celebration being moved (as in Latin America)? Especially in cases of moving to Monday from other working days. Maybe we should use _move_holiday()?

@PPsyrius
Copy link
Contributor Author

PPsyrius commented Oct 30, 2023

I have some doubts about the exact meaning of Tonga observed holidays. Is it just an extra day off or is the actual celebration being moved (as in Latin America)? Especially in cases of moving to Monday from other working days. Maybe we should use _move_holiday()?

I'm rechecking this at the moment, considering the lack of information in general.

The gov website dictates that Nov 4, 2023 - the actual date itself, is also an holiday, at least for Tonga's Ministry of Communications for example, though other evidence i.e. Matangi Tonga Online does indeed points towards LatAm implementation for 2021-2022 for the general public.

@arkid15r @KJhellico Please let me know which way implementation better aligns with actual situation here 👀

@PPsyrius PPsyrius marked this pull request as ready for review October 30, 2023 06:36
@PPsyrius
Copy link
Contributor Author

PPsyrius commented Oct 31, 2023

Update: I'm going with the move_holiday method for now, then adding them back if those three holidays fall on their actual date for GOVERNMENT category - seems like the best compromise for this one, which fits both public and gov calendar available online.

Edit: simply picking the non-observed option would've gotten the same effect as what we're looking for in the government calendar, thus not implemented.

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 in general, here are my readability/consistency improvement suggestions:

holidays/countries/tonga.py Outdated Show resolved Hide resolved
holidays/countries/tonga.py Outdated Show resolved Hide resolved
holidays/countries/tonga.py Outdated Show resolved Hide resolved
holidays/countries/tonga.py Outdated Show resolved Hide resolved
holidays/countries/tonga.py Outdated Show resolved Hide resolved
tests/countries/test_tonga.py Outdated Show resolved Hide resolved
PPsyrius and others added 6 commits November 1, 2023 01:19
Co-authored-by: Arkadii Yakovets <ark@cho.red>
Co-authored-by: Arkadii Yakovets <ark@cho.red>
Co-authored-by: Arkadii Yakovets <ark@cho.red>
Co-authored-by: Arkadii Yakovets <ark@cho.red>
Co-authored-by: Arkadii Yakovets <ark@cho.red>
Co-authored-by: Arkadii Yakovets <ark@cho.red>
Copy link

sonarcloud bot commented Oct 31, 2023

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 12 Code Smells

No Coverage information No Coverage information
No Duplication information No Duplication information

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

@arkid15r arkid15r added this pull request to the merge queue Nov 2, 2023
Merged via the queue into vacanza:beta with commit 208ea97 Nov 2, 2023
27 checks passed
@PPsyrius PPsyrius deleted the tonga_public_holidays branch November 3, 2023 11:08
@arkid15r arkid15r mentioned this pull request Nov 6, 2023
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.

Add Tonga holidays
4 participants