Skip to content
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

Holidays that fall on a Sunday are omitted from the calendar. #71

Open
adamhassel opened this issue Jun 30, 2021 · 6 comments
Open

Holidays that fall on a Sunday are omitted from the calendar. #71

adamhassel opened this issue Jun 30, 2021 · 6 comments

Comments

@adamhassel
Copy link

adamhassel commented Jun 30, 2021

There seems to be an assumption in the code that holidays that always fall on a Sunday, are not included in the holiday calendars. The most obvious example is Easter Sunday.

This means that if a business has Sunday configured as a Working Day, then Easter Sunday is not considered a holiday. This would be wrong for e.g. the majority (if not all) countries, that consider Easter Monday a holiday.

For example, stores, that are generally open on Sundays, could be closed on Easter Sunday.

Is this by design, or simply an oversight? It seems like a general theme, because even though aa.EasterMonday is defined, there seems to be no general definition of Easter itself. The same goes with Whit Sunday, and possibly others..

@rickar
Copy link
Owner

rickar commented Jun 30, 2021

That is the convention I went with, yes. The default is set to workdays being Monday through Friday and the default holiday sets match that. I was assuming that if you were going to override the workdays then you would probably also not be using the default holiday sets anyway. For example, a lot of retail stores in the US will only take 1-2 holidays if any at all while other businesses will take all the ones defined at the federal level.

The weekend holiday definitions can definitely be added. Adding them to the default list would be a breaking change at this point though.

@adamhassel
Copy link
Author

adamhassel commented Jul 1, 2021

I see. Well, for many places other than the US, this is not really an abnormal convention. Rather, I'd go so far as to call it a bug that it's not working this way :)

How would it be a breaking change to add them to the default list for locations that use them (which is at least most of, if not all of Europe)? And/or at least have them defined in the aa calendar for easy inclusion?

Edit: Also, I see that it's actually already defined for at least Australia. So why not others?

@rickar
Copy link
Owner

rickar commented Jul 1, 2021

Ah, living in the modern dystopia strikes again. ;-)

v2 was released partially because I had made assumptions about how holidays on weekends were handled that turned out to be US-specific. I took the opportunity to make some more significant refactors too so it was worth it.

The idea was to be able to support all types of holidays (religious, state/region specific, special observances and anniversaries, etc.) so it's not even necessary that the holiday results in a business closure, but I hoped that the default holidays array for each country would be useful for the most common cases.

The holidays that exist in the code already were mostly contributed by others (who I assume in many cases actually lived in those regions). So there are different levels of detail depending on what was submitted as pull requests. Australia is probably one of the most complete ones.

There's nothing wrong with adding Easter to aa for example, and I would expect those common holiday definitions to be there. The breaking change would be to update all the country specific Holidays arrays because existing apps could be using those and either expecting Easter not to be there or adding their own instance for example.

@adamhassel
Copy link
Author

I think it's probably a bad strategy to not fix things because some people may or may not rely on the behavior being wrong.

I'd argue that at least aa.EasterSunday and aa.Pentecost could be added with little or no ill effect to most other (European) countries...

@rickar
Copy link
Owner

rickar commented Jul 1, 2021

I was agreeing with you... aa.EasterSunday, aa.Pentecost, aa.XYZ, ... can all be added without affecting anyone.

I'm just saying that changing ca.Holidays, nl.Holidays, etc. would be a breaking change and require a v3 release.

@quite
Copy link
Contributor

quite commented Jun 24, 2022

I have MR #94 that adds Easter and Pentecost to aa, and as well adds these to Swedish.

In Sweden, these are both part of allmänna helgdagar "common holidays". It doesn't matter if these fall on Sundays or not. From the Swedish perspective, I'd expect an array with these ones, as they are the only one's which are basically coded in law. There are currently 13 of them, but these also change over time. It's politics, union negotiations and whatnot.

In particular, se's current Holidays slice contains 3 aftnar "evenings", which are not common holidays. It doesn't matter if some choose to let their employees off on some of these days -- they are not common holidays. Perhaps it could be considered de facto, but not de jure. The slice is also missing Pingstdagen "Pentecost Sunday", which falls on a Sunday and is a "common holiday" nevertheless. So here we come back to the topic of this issue. I think that holidays that falls on Sundays should be part of Holidays. But I understand that altering that slice might be problematic, and might require a version bump. So I'm not making that change in the MR.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants