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

Adding German holidays #8

Merged
merged 13 commits into from
Jan 25, 2024
Merged

Adding German holidays #8

merged 13 commits into from
Jan 25, 2024

Conversation

arnebr
Copy link
Contributor

@arnebr arnebr commented Jan 17, 2024

This uses the nation-wide holidays for Germany from https://de.wikipedia.org/wiki/Gesetzliche_Feiertage_in_Deutschland

Regional state holidays are not part implemented in the library. Maybe @freekmurze as future plan?

@benjibee
Copy link

I was just about to open this PR! I also asked on Twitter about adding regional public holidays. Looking at the code it would be quite a big change to specify the region alongside the country.

@arnebr
Copy link
Contributor Author

arnebr commented Jan 17, 2024

For easy implementing we could use something like de-sh but that's not "normed".
For Austria it would be the same. We could look at libraries like: https://github.com/umulmrum/holiday

@arnebr
Copy link
Contributor Author

arnebr commented Jan 17, 2024

Notes on German Holidays

  • Easter Sunday/Ostersonntag and Whit Sunday/Pfingstsonntag are not public holidays in most states.
  • Every Sunday is a public holiday in Hesse.
  • Assumption/Mariä Himmelfahrt is a public holiday in about 1700 of about 2000 communities in Bavaria. This is
    implemented as a partial holiday, so you might want to add your logic that filters this date.
  • In Bavaria schools are closed on Assumption Day and on Repentance and Prayer Day. There is no special handling of
    this "behavior".
  • Corpus Christi/Fronleichnam is a public holiday in some communities in Saxony and Thuringia.
  • Regional holidays with traditionally (but not publicly) limited opening hours are not considered yet (e.g. carnival
    days).
  • Some ports at the German Ocean celebrate some holidays as "High Holidays". It is generally not allowed to work on
    these days, and work time ends at 12 o'clock the day before. This is not considered yet.
  • Also there are some quiet days that are not public holidays in various states. This is also not considered yet.
  • New states of Germany are treated as if they had always been part of the Federal Republic of Germany. For years
    before 1990 holidays are therefore not correct.

source: https://github.com/umulmrum/holiday

@Nielsvanpach
Copy link
Member

Nielsvanpach commented Jan 17, 2024

Regional state holidays are not part implemented in the library. Maybe freekmurze as future plan?

We are open to support this, feel free to open a PR.
Maybe the Country class can be extended with a region property, which could be used in allHolidays() ?

As this is also discussed on the Indian PR, I've added it as feature request: #14 to have a single discussion on it.

@Nielsvanpach
Copy link
Member

I've added documentation on how I think we should manage regions or states:
https://github.com/spatie/holidays?tab=readme-ov-file#contributing-a-new-country

Could you get this PR up to date?

@arnebr
Copy link
Contributor Author

arnebr commented Jan 18, 2024

Will do 😊

@arnebr
Copy link
Contributor Author

arnebr commented Jan 18, 2024

@Nielsvanpach I think #37 superseeds this PR, since it's implemented all states 😊

@marlonbasten
Copy link

marlonbasten commented Jan 19, 2024

@arnebr As my PR won't get merged, there's still a need for German state holidays. 😕 Feel free to add this to your PR! I don't think that this package has a good implementation of it as of now, but we have to work with what we've got I guess. The package mentioned a few comments above has a good implementation with states and holiday types.

@arnebr
Copy link
Contributor Author

arnebr commented Jan 21, 2024

Maybe @marlonbasten you could do a review of the holidays :) I implemented all of them now.
@Nielsvanpach hope that´s the way you would like to see it.

Copy link
Member

@Nielsvanpach Nielsvanpach left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

PR looks good, thanks!

expect(formatDates($holidays))->toMatchSnapshot();
})->with(
[
['BW',12],
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Have you actually checked the results in the snapshots? Otherwise having only 1 region test is fine.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yes thats why i found the error in the SN,BB, BE state :)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great, thanks for this!

Copy link
Member

@Nielsvanpach Nielsvanpach left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

PR looks great, thanks for your work! 👍

@Nielsvanpach Nielsvanpach merged commit ab04ec4 into spatie:main Jan 25, 2024
8 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants