-
-
Notifications
You must be signed in to change notification settings - Fork 158
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
Added support for Germany #9
Conversation
@@ -19,5 +19,6 @@ | |||
'da_DK' => 'Kristi Himmelfartsdag', | |||
'nb_NO' => 'Kristi himmelfartsdag', | |||
'sv_SE' => 'Kristi himmelsfärds dag', | |||
'fi_FI' => 'Helatorstai' | |||
'fi_FI' => 'Helatorstai', | |||
'de' => 'Christi Himmelfahrt' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Although 'de' is an official locale, it is better to use a locale that identifies also the region as well. In this case 'de_DE' is preferred. Can you adjust your PR accordingly?
Thank you very much for the PR @eaglefsd! Very much appreciated. I left a few comments on your PR. Also as part of the unit tests, there should also be a Test.php (see for example Yasumi/tests/Finland/FinlandTest.php). This test contains tests to confirm all the holidays that you expect for this country. In the future, I would like also to see the individual German States' holidays as they differ from the national ones; in the same way as done in Spain. If you could review your PR based on my comments, I can merge it afterwards. |
Well, I used 'de' because you used it in the locales :) |
Hi, I can commit the different states with their holidays, but I don't have the time to write the tests at the moment :/ |
That would be great! I can do the unit tests for you. In that case I prefer you prepare the PR for just Germany without the state holidays first. I would like to do then a next update for the German state holidays. |
I removed the states in my latest commit, do I have to do anything else? |
Sounds good. I will merge it and if something needs to be altered we can do that. |
Not that familiar with open source and tests, if I'm missing something, just let me know!