azuyalabs / yasumi

The easy PHP Library for calculating holidays
https://www.yasumi.dev
Other
1.04k stars 152 forks source link

Adds Slovakia #40

Closed dakujem closed 7 years ago

dakujem commented 8 years ago

Adds holiday support for Slovakia

Holidays in Slovakia are divided into bank holidays and national days.

stelgenhof commented 8 years ago

Thanks for the contribution! Let me have a look at the PR in the next few days :)

dakujem commented 8 years ago

yw. no idea why the checks are failing. The unit tests passed on 5.6.

stelgenhof commented 8 years ago

The unit test themselves are not failing. There is an additional check for PSR-2 coding standards which is failing. Please run composer php-cd-fixer before submitting your PR :)

stelgenhof commented 8 years ago

For Slovakia, are there any holidays that have been established from a certain date or year? If so, it is good to add a condition to the calculation when such a holiday became in effect. Likewise if a holiday has been abolished. This allows for other users to still use past/new holidays for their use case.

Apart from the coding style, it all looks good!

dakujem commented 8 years ago

From my knowledge, since 1993 when Slovakia became a souvereign country there have been no changes. Before we were a part of Czecho-Slovakia, I don't know how this should be handled. There were quite a few changes in the first part of the 20-th century.

dakujem commented 8 years ago

Yay, I ran the cs fixer, but the build is still failing. I'm working on Windows, maybe that is a problem?

stelgenhof commented 8 years ago

If technically speaking the holidays didn't exist before the time Slovakia and Czech Republic became sovereign countries, I would then implement that the Slovakian holidays only are generated after 1993. This would account for Czech Republic then as well. (Before 1993 we should have a separate holiday provider for Czechoslovakia).

As for your errors, I don't use Windows so can't really help there. All is mostly built on non Windows platforms. At the end I can merge your PR and fix it on my side for you.

Cheers! Sacha

dakujem commented 8 years ago

Well, then it would become quite complex - there would have to be at least 5 more providers for 20-th century only... As Slovakia was part of the Autro-Hungarian monarchy, then Czechoslovakia, then independent and separated, then Czechoslovakia again, then another Czechoslovakia, yet another, until finally independent since 1993. Not to mention changes in borders. I think that it's not the aim of the library to reflect history.

I suggest I check for the holidays celebrated since say end of WWII in the area of current Slovakia and incorporate them into the provider, provided I can find relevant information.

What is your opinion?

stelgenhof commented 8 years ago

I agree that for the most part, future holidays are used. However there are some use cases where past holidays are required in certain calculations for example. In the Netherlands for example the national Queensday was changed date a few times and recently renamed to "Kingsday".

Although not indeed extremely important I would suggest for those holidays you know it only started from a specific year, implement the starting year. Otherwise leave it as is :)

dakujem commented 8 years ago

Actually, I was unable to find any reliable information source of holidays before 1993. Some holidays were celebrated on the particular day with changed names, but from what I found, before 1989 there were many and much different holidays, since it was a different regime... So... it probably would require implementing separate provider anyway.

Edit: I forgot to note, that I specifically mention in the documentation of the provider class, that the holidays are only valid starting from 1993, which is the year when Slovakia became a sovereign state..

stelgenhof commented 8 years ago

@dakujem Ok. Understood, makes sense. I'll merge your PR. It is always possible to address these situations at a later stage. Thanks again for the contribution!