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 New Zealand #13

Merged
merged 21 commits into from
Apr 9, 2016
Merged

Added support for New Zealand #13

merged 21 commits into from
Apr 9, 2016

Conversation

badams
Copy link
Contributor

@badams badams commented Apr 6, 2016

Adding support for New Zealand national holidays.

@badams badams changed the title New zealand holiday provider (WIP) Added support for New Zealand Apr 6, 2016
@stelgenhof
Copy link
Member

Thanks for the support of adding New Zealand! Looking forward to the final PR 👍

@badams
Copy link
Contributor Author

badams commented Apr 6, 2016

Thanks @stelgenhof I saw your post on /r/php and liked the library, so I wanted to contribute to improve the coverage.

My plan is to finish off the national holidays for NZ, then I'll create a new PR for provincial holidays. If you want, and I have time I'll start another PR for Australia support later.

I'd appreciate if you could review what I've done so far and make sure I've not deviated from the goals of the project, I've added Mondayisation to all the applicable holidays

@stelgenhof stelgenhof added this to the 1.3.0 milestone Apr 6, 2016
@stelgenhof
Copy link
Member

Thanks @badams! Let me have a look later this week. I will then merge it with the base branch. I have set this as a milestone for the 1.3.0 version release :)

@badams
Copy link
Contributor Author

badams commented Apr 7, 2016

Sounds good to me @stelgenhof 👍

*/
public function HolidayDataProvider()
{
return [
Copy link
Member

Choose a reason for hiding this comment

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

Wow! Nice list... Would it be possible to generate the dates rather than hardcoding them? Would be more sustainable I think :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@stelgenhof These lists are actually generated, I didn't write them by hand don't worry :)
My reasoning for hard coding them was that the code used to generate them seemed a bit too complex for unit tests, it resembled the calculation methods in the provider; to me it made sense to keep the tests as simple as possible.

But that's no problem I can use the code I generated the lists with instead.

Merge branch 'master' into new_zealand_holiday_provider
['1929', '1929-04-25'],
];
$data = [];

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@stelgenhof does this look okay?

Copy link
Member

Choose a reason for hiding this comment

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

Yes, looks great! BTW There have been some new commits after your created this PR, so might want to sync up your PR :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok I'll update the rest of the tests

@badams
Copy link
Contributor Author

badams commented Apr 7, 2016

@stelgenhof I've adjusted all the tests as requested, let me know if anything else :)

@stelgenhof stelgenhof merged commit 7507e45 into azuyalabs:master Apr 9, 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.

2 participants