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

Australian Public Holidays #112

Merged
merged 2 commits into from Jun 26, 2018
Merged

Conversation

Milamber33
Copy link
Contributor

Proper handling for (just about) all public holidays in every Australian state and territory.

Proper handling for (just about) all public holidays in every Australian state and territory.
@stelgenhof stelgenhof self-requested a review June 21, 2018 12:40
@stelgenhof
Copy link
Member

Thanks! Sorry for not replying sooner. There is no need to close PR's if something needed to change.
I'll review your PR and merge it if it looks good :)

@stelgenhof stelgenhof added this to the v2.0.0 milestone Jun 21, 2018
/**
* Returns a list of test dates
*
* @return array list of test dates for the holiday defined in this test
*/
public function HolidayDataProvider(): array
public function HolidayDataProvider()
Copy link
Member

Choose a reason for hiding this comment

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

Yasumi is supporting PHP7 going forward, so please use/keep any return type declarations.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure, but this is probably going to be needed in a lot of the other ones I've written. I copied and pasted a lot. I'll go through and try to get them all.

*
* @throws \Yasumi\Exception\InvalidDateException
* @throws \InvalidArgumentException
* @throws \Yasumi\Exception\UnknownLocaleException
* @throws \Exception
*/
public function calculateHoliday(
string $shortName,
$shortName,
Copy link
Member

Choose a reason for hiding this comment

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

Yasumi is supporting PHP7 going forward, so please use/keep any return type declarations.

* @param bool $moveFromSunday
* @param string $shortName
* @param array $names
* @param string|DateTime $date
Copy link
Member

Choose a reason for hiding this comment

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

Please use the DateTime type only.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Okay, that... breaks a lot. It will take me a few hours to make all the other changes to stop using strings everywhere.

Copy link
Member

@stelgenhof stelgenhof left a comment

Choose a reason for hiding this comment

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

Left some comments. Can you also add an entry in the CHANGELOG.md file that describes the changes made in this PR?

Fixes requested for PHP7 compliance.
@Milamber33
Copy link
Contributor Author

Okay, got those ones you pointed out, and all the other instances where they were copied. There could well be more missing type declarations, I was working based off the current release when adding the extra files, and I used copy+paste a lot. Just let me know if you see any and I'll deal with them. :)

@stelgenhof
Copy link
Member

Thanks a lot!

My apologies if I haven't been clear. Indeed the current stable release supports PHP 5.6 and greater. The development branch only supports PHP7 and greater and will be the basis for the next v2.0 release. I haven't documented/communicated that properly (yet).

@Milamber33
Copy link
Contributor Author

No problems. :)

@stelgenhof stelgenhof merged commit bc1b690 into azuyalabs:develop Jun 26, 2018
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