-
Notifications
You must be signed in to change notification settings - Fork 470
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
Update Spain holidays #1476
Update Spain holidays #1476
Conversation
Pull Request Test Coverage Report for Build 6260031150
💛 - Coveralls |
d439068
to
8722cd6
Compare
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.
Nicely done! I believe the covered holiday dates precision will be drastically increased!
I noticed you're using (I guess intentionally) multiple identical ifs for different (chronologically within a year) holidays: if self._year in {2013, 2019}:
, if self._year in {2015, 2020}:
Would it be possible to combine those or the current order is absolutely necessary for the resulting holiday list? If that's just a readability choice we should consider readabiity/performance tradeoffs before applying this approach widely.
Also some minor comments, mostly naming:
No, order is not important, but in some of the following years one of these holidays may exist (in this province), and other may not. And |
Proposed change
Update Spain holidays (checked with official sources for 2010-2023).
Type of change
Checklist
beta
branch of the repositorymake pre-commit
command generates no changesmake test
,make tox
(we strongly encourage adding tests to your code)