-
Notifications
You must be signed in to change notification settings - Fork 462
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
Refactor HolidayBase class and utils #815
Conversation
- Optimize branching. - Remove unneeded casting/copying. - Add `_add_observed_holiday` method. - Optimize methods: - `__setattr__` - `__keytransform__` - `__getitem__` - `__setitem__` - `get_named` - `update` - `__repr__` - `__str__` - Simplify `utils.country_holidays` country/market lookup logic. - Fix minor issues (style, grammar, punctuation). - Clean up unused code.
Wow, that's a lot of good edits. 👍 I can't find any apparent faults with it, but realize that while I wrote util._islamic_to_gre to return a list, it could/should probably return a generator instead. |
Thanks for the review, @mborsetti!
That's a good point, done! |
So good work! I could not check every single row, but all looks fine to me! Thank you @arkid15r 👍 |
- fix for holidays that happen twice in a Gregorian year (primarily islamic New Year) - years range checking more strictly - utils._islamic_to_gre() refactoring from vacanza#815 (except returning value type change)
This PR aims to simplify and optimize PH foundational code logic.
It covers
holiday_base.py
andutils.py
. The main points are:_add_observed_holiday
method.__setattr__
__keytransform__
__getitem__
__setitem__
get_named
update
__repr__
__str__
utils.country_holidays
country/market lookup logic.I'd like to request @dr-prodigy (obviously), @kasya, @KJhellico, @mborsetti consider reviewing it. Feel free to jump in even if you weren't tagged but have comments regarding this change.
Thank you!