-
-
Notifications
You must be signed in to change notification settings - Fork 560
Refactor holiday categories handling #1584
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
Conversation
- 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
Pull Request Test Coverage Report for Build 7136383310
💛 - Coveralls |
There was a problem hiding this 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 🎉
There was a problem hiding this 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.
There was a problem hiding this 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
|
Kudos, SonarCloud Quality Gate passed!
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM! 🚀








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_categoryattribute 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:
has_special_holidaysandhas_substituted_holidaysas permanentHolidayBaseattributes_populateprefix for high-level methods (common/subdivision holidays)Type of change
Checklist
make pre-commit, it didn't generate any changesmake test, all tests passed locally