python-holidays icon indicating copy to clipboard operation
python-holidays copied to clipboard

Holiday names sorting

Open kasya opened this issue 2 years ago • 1 comments

While I was working on Bolivia holidays one of my tests caught an error with holiday names when two holidays fall on the same day. In this particular case it was one regional holiday (La Tablada in Tarija) and Viernes Santo holiday. In 2022 they both fell on Apr 15, so I had to make a separate test for this case. But to test it I needed to know the logic behind how names are combined.

I checked the code and noticed that there was no "rule" for holiday names order in such cases, and that makes testing much harder.

@dr-prodigy Would it be ok to submit a PR with a change to this fundamental logic?

kasya avatar Sep 14 '22 02:09 kasya

Hi @kasya thank you for this interesting case! Indeed, we don't have a clear definition of behaviour in case of holidays overlapping.. being holidays a Dict-based class, we can't in fact add more than one holiday on the same date right away. Then, it would surely be valuable to add a standard logic to cover those cases: it would be useful especially regarding moving holidays (eg: Easter, Good Friday, etc.). My suggestion, whenever it happens, is, while keeping one single date element, to define a standard format to include all the holiday descriptions: they could be comma/semicolon separated, or new lines. This will probably imply some country holidays code refactoring too (I remember we have some custom logic regarding this in the project already), but that will come probably later.. Please, go on.. I'll be glad to participate to this development and include it in a PR. Thank you!

dr-prodigy avatar Sep 14 '22 09:09 dr-prodigy

Is this still an issue, or has #713 closed/solved this? :)

FirefoxMetzger avatar Oct 11 '22 15:10 FirefoxMetzger

Is this still an issue, or has #713 closed/solved this? :)

#713 solved this issue (holiday names are now sorted alphabetically when 2 or more holidays fall on the same date).

kasya avatar Oct 13 '22 00:10 kasya

Confirm.. PR released in beta. Closing this, thx everyone!

dr-prodigy avatar Oct 19 '22 09:10 dr-prodigy