Skip to content

Conversation

@arkid15r
Copy link
Collaborator

@arkid15r arkid15r commented Dec 5, 2023

Proposed change

This PR contains work towards addressing some concerns raised in #1559 by making holiday categories naturally integrated into holidays population logic (opposite of ad-hoc approach that was used when they were introduced initially).

It adds nullable HolidayBase::default_category attribute which defaults to 'PUBLIC' category at this moment. The default_category=None use case needs to be thought through -- please to add your input if any.

As holiday category blend into the base class structure the main focus remains on entity common and per subdivision holidays. All attributes/methods related to public holidays are now named explicitly (contain _public_ in theirs name).

Other changes:

  • improve base class init args validation
  • refactor observed holidays base class according to new parent's class structure
  • refactor has_special_holidays and has_substituted_holidays as permanent HolidayBase attributes
  • use _populate prefix for high-level methods (common/subdivision holidays)
  • allow spaces in subdivision names
  • update README, remove PUBLIC from supported categories column as it's a default one supported by all countries

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 run make pre-commit, it didn't generate any changes
  • I've run make test, all tests passed locally

  - Implement default category support
  - Refactor `HolidayBase` holidays population methods to handle
    categories internally focusing on common/subdiv structure only
  - Add PUBLIC to `HolidayBase::supported_categories`
  - Rename `HolidayBase::_add_subdiv_holidays` to
    `HolidayBase::_populate_subdiv_holidays`
  - Use default category in special holidays dict name
  - Add `has_special_holidays` and `has_substituted_holidays` as
    `HolidayBase` attributes
  - Improve `HolidayBase` init arguments validation
  - Allow spaces in subdivision names
@arkid15r arkid15r requested a review from KJhellico as a code owner December 5, 2023 20:41
@arkid15r arkid15r requested review from KJhellico and PPsyrius and removed request for KJhellico December 5, 2023 20:42
@coveralls
Copy link

coveralls commented Dec 5, 2023

Pull Request Test Coverage Report for Build 7136383310

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

Totals Coverage Status
Change from base Build 7133123831: 0.0%
Covered Lines: 10307
Relevant Lines: 10307

💛 - Coveralls

Copy link
Collaborator

@PPsyrius PPsyrius left a comment

Choose a reason for hiding this comment

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

LGTM and nice job with the standardization 🎉

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.

It's great! I hope my few suggestions will be relevant.

Copy link
Collaborator Author

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

Updated it based on the comments

@arkid15r arkid15r requested a review from KJhellico December 6, 2023 19:01
@sonarqubecloud
Copy link

sonarqubecloud bot commented Dec 8, 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 22 Code Smells

No Coverage information No Coverage information
0.0% 0.0% Duplication

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! 🚀

@arkid15r arkid15r added this pull request to the merge queue Dec 8, 2023
Merged via the queue into vacanza:beta with commit 083ed9a Dec 8, 2023
@arkid15r arkid15r deleted the extend-holiday-categories branch December 8, 2023 17:59
@arkid15r arkid15r mentioned this pull request Dec 18, 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.

4 participants