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 Holiday Provider for Brazil #21

Merged
merged 9 commits into from May 23, 2016
Merged

Conversation

dorianneto
Copy link
Contributor

No description provided.

@stelgenhof
Copy link
Member

Thank you @dorianneto ! Very nice, however it has some conflicts with the current branch. Can you check again?

@dorianneto
Copy link
Contributor Author

Checked and validated :)

I'm sorry for a lot of commits. I forgot to use the php-cs-fixer before make push :P

@stelgenhof stelgenhof added this to the v1.4.0 milestone Apr 26, 2016
@dorianneto
Copy link
Contributor Author

@stelgenhof ?

@stelgenhof
Copy link
Member

@dorianneto Not sure what you mean, though :) I still need to check your PR; planning this for the 1.4 version. Once I have finished 1.3 I will merge it into the master branch. In case you need it earlier, you can still use your own fork.

* Timezone in which this provider has holidays defined
*/
const TIMEZONE = 'America/Fortaleza';
}
Copy link
Member

Choose a reason for hiding this comment

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

Please add a constant for the common locale in Brazil. See for example the other test classes ( const LOCALE = 'nl_NL';)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done!

@dorianneto
Copy link
Contributor Author

All done :)

@stelgenhof
Copy link
Member

Thanks @dorianneto ! Looking good. Let me review once more and merge it soon.
Appreciate the PR!

@stelgenhof
Copy link
Member

Just ran another unittest and found some errors:

1) Yasumi\tests\Brazil\BrazilTest::testNationalHolidays
Failed asserting that an array has the key 'tiradentesDay'.

If a holidays is of a certain type (e.g. National), make sure they are added in the respective test of the BrazilTest class.

Running a single iteration of PHPUnit might not detect all issues. Please run with --repeat 10 for example. You might see more issues appearing.

'goodFriday',
'easter',
'diaDeTiradentes',
'tiradentesDay',
Copy link
Contributor Author

Choose a reason for hiding this comment

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

The holiday is in the correct array.

@dorianneto
Copy link
Contributor Author

Before push this commit, I verified all unittests and not found no problem. Now, I ran with --repeat 10 and sometimes have a problem sometimes not.

This is a PHPUnit bug or I have to do anything? I never experienced this problem, sorry :(

@stelgenhof
Copy link
Member

The reason for that is, that the unit tests use a randomized year. It might be possible that for that particular year the respective holiday doesn't exist. Usually this becomes apparent when running multiple iterations :). Nothing to worry about. Let me merge it and fix it for you.

@stelgenhof stelgenhof merged commit aab9f88 into azuyalabs:master May 23, 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