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

Added support for Germany #9

Merged
merged 5 commits into from
Apr 6, 2016
Merged

Added support for Germany #9

merged 5 commits into from
Apr 6, 2016

Conversation

eaglefsd
Copy link
Contributor

@eaglefsd eaglefsd commented Apr 4, 2016

Not that familiar with open source and tests, if I'm missing something, just let me know!

@@ -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'
Copy link
Member

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?

@stelgenhof
Copy link
Member

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.

@eaglefsd
Copy link
Contributor Author

eaglefsd commented Apr 5, 2016

Well, I used 'de' because you used it in the locales :)
But I can change it, sure.
Yes, good Friday is a day where the shops are closed, we even have a rule which forbids music in clubs.
I didn't have the time yesterday for the individual parts, but I will definetely have a look later today!

@eaglefsd
Copy link
Contributor Author

eaglefsd commented Apr 6, 2016

Hi, I can commit the different states with their holidays, but I don't have the time to write the tests at the moment :/

@stelgenhof
Copy link
Member

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.

@eaglefsd
Copy link
Contributor Author

eaglefsd commented Apr 6, 2016

I removed the states in my latest commit, do I have to do anything else?
The translations for the state holidays (like epiphany) are still present tho, I hope you don't mind...

@stelgenhof
Copy link
Member

Sounds good. I will merge it and if something needs to be altered we can do that.

@stelgenhof stelgenhof merged commit 0a0e59d into azuyalabs:master Apr 6, 2016
@stelgenhof stelgenhof added this to the 1.3.0 milestone Apr 6, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants