vacanza
Introduce special holidays support
Extend HolidayBase in order to make special holidays handling simpler.
The idea behind this is to simplify adding country-wide one-off holidays for all countries. As the most recent example I saw a bunch of pretty similar PRs for Australia, New Zeland, UK (Canada too?) adding special holidays related to the death of Queen Elizabeth II.
If the change is accepted the next step will be to migrate the rest of the countries to the new approach.
Coverage remained the same at 100.0% when pulling 70e0dfe4260ff46248075a12f8c7763b40421885 on arkid15r:special-holidays-refactoring into 4b83875321cff0778b58a9eada462d94ce46b3cd on dr-prodigy:beta.
Hi Ark! Although it is not a bad idea and working well:
- in my knowledge, those special holidays, especially to be added to a number of countries in one time, are a very rare occurrence (up to now, Queen Elizabeth's celebrations is the only case I remember of)
- this new way of coding holidays diverges heavily from the "standard" one and, apart in the case of a large number of single holidays to be added to the same country (which is pretty unusual as well: we normally focus on recurring holidays), it even requires more code lines than the usual method.
For these reasons, at the moment, I would avoid to merge it as it is.
I would anyway reconsider this suggestion if we are able to extend this to a broader number of cases: let's discuss this if you wish! 👍
Thank for your understanding, let's keep posted on this!
Hi Mauri, Thanks for the review!
Yeah, you're absolutely right about how rare those special holiday occur. This PRs tends to separate all non-recurring holidays from "standard" holidays with more complex logic defined in _populate (quoting your words: "we normally focus on recurring holidays"). After a quick research I found about 10-15 classes that could use the new structure. Please note that current PR covers only a part of them.
Here are some points to defend my intent and get this PR submitted:
- significantly simplify one-off holidays adding process
- decrease code clutter
- improve code reusing/readability
it even requires more code lines than the usual method.
I'd like to request more details about this comment. Could you elaborate?
I would anyway reconsider this suggestion if we are able to extend this to a broader number of cases: let's discuss this if you wish! 👍
I'm open to any suggestions to implement/rework in order to get this PR accepted.
I've listed the points why this PR would be good and can't find a good reason for not accepting it. I'm looking forward to address any other concerns you may have.
P.S. after another look I plan to get rid of _populate_special_holidays method and put its logic into _populate method making sure the HolidayBase derived classes invoke parent method. In this way the special holidays logic will be completely under the hood.
Hi Ark @arkid15r , thank you for your notes! At a further analysis, I agree with you: it could be a beneficial improvement, so we can merge it. On the other hand, I've noticed a piece of code (currently conflicting, btw) on UK: it seems your new UK code references IsleOfMan, which in turn is a subclass of UnitedKingdom.. I would rather avoid this kind of cross-references between classes: can you refactor to remove it? Thanks!
Hey Mauri, thanks for reconsidering this!
I'll look into the conflicting code later today. It seems to be a piece of pre #747 PR (see here) and it should be resolved after I merge beta's recent changes. In a nutshell, it was removed in my other PR and I'll sync it today.