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.

Sign up for GitHub

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

Jump to bottom

Clean up calendars #1349

Merged
merged 2 commits into from
Jun 30, 2023
Merged

Clean up calendars #1349

merged 2 commits into from
Jun 30, 2023

Conversation

arkid15r
Copy link
Collaborator

Proposed change

Clean up calendars

  • Extract Gregorian calendar related stuff to calendars/gregorian.py
  • Add calendars/julian.py for JULIAN_CALENDAR only
  • Remove redundant KHMER_CALENDAR and THAI_CALENDAR constants

This PR is a preparation step for a wider HolidayBase::_add_holiday refactoring.

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)

@arkid15r
Clean up calendars
484703c 
  - Extract Gregorian calendar related stuff to `calendars/gregorian.py`
  - Add `calendars/julian.py` for `JULIAN_CALENDAR` only
  - Remove redundant `KHMER_CALENDAR` and `THAI_CALENDAR` constants
@arkid15r arkid15r requested a review from KJhellico June 29, 2023 21:02
@coveralls
Copy link

coveralls commented Jun 29, 2023 •
edited
Loading

Coverage Status

coverage: 100.0%. remained the same when pulling a19645d on arkid15r:clean-up-calendars into 81ee3d6 on dr-prodigy:beta.

KJhellico
KJhellico reviewed Jun 30, 2023
View reviewed changes
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.

holidays/constants.py

MON, TUE, WED, THU, FRI, SAT, SUN = range(7)
WEEKEND = (SAT, SUN)
from holidays.calendars.gregorian import (
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why are we keeping here these constants import? For external compatibility?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

That's right, it needs to stay backward compatible

@arkid15r arkid15r added the ready for beta Ready to merge on beta branch label Jun 30, 2023
@KJhellico KJhellico merged commit ecf7c15 into vacanza:beta Jun 30, 2023
@KJhellico KJhellico removed the ready for beta Ready to merge on beta branch label Jun 30, 2023
@KJhellico KJhellico mentioned this pull request Jul 3, 2023
Merged
@arkid15r arkid15r deleted the clean-up-calendars branch July 7, 2023 01:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@KJhellico KJhellico KJhellico left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@arkid15r @coveralls @KJhellico