-
Notifications
You must be signed in to change notification settings - Fork 487
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
Support for holidays of multiple countries #1001
Support for holidays of multiple countries #1001
Conversation
β¦lidays of multiple countries are working
Codecov Report
@@ Coverage Diff @@
## main #1001 +/- ##
==========================================
+ Coverage 90.26% 90.28% +0.01%
==========================================
Files 21 21
Lines 4736 4744 +8
==========================================
+ Hits 4275 4283 +8
Misses 461 461
Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. |
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.
Great work, looks very good to me overall. I've added a few minor suggestions and there are a few flake8 warnings which you should look at (one of them is actually critical as the variable name is not defined). Nothing big, you're on a good track, keep it up π
Co-authored-by: Richard Stromer <[email protected]>
corrected type in docstring Co-authored-by: Richard Stromer <[email protected]>
β¦ for consistent naming
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 - I've added two minor suggestions which I'd add before merging and one note about duplicate code which we could extract to a helper function
consistent naming of types in docstring Co-authored-by: Richard Stromer <[email protected]>
consistent naming of types string -> str Co-authored-by: Richard Stromer <[email protected]>
Model Benchmark
|
π¨βπ¬ Status quo
Currently it is only possible to consider holidays of one country. If another country is added, the previous one is overwritten. Addresses issue #763
π¨βπ¨ Key changes
It is possible now to add a list of countries, whose holidays should be considered. For example m.add_country_holidays(country_name = ["US", "Germany"]) will consider holidays of the US and Germany. For redundant holidays (here: 'Christmas Day' and 'Erster Weihnachtstag') the name of the last mentioned country will be kept (here: 'Erster Weihnachtstag')
π΅οΈββοΈ Review Checklist