Skip to content

Conversation

ovgray
Copy link
Contributor

@ovgray ovgray commented Aug 29, 2021

This PR fixes the definition of Canada Day in the Canada provider and adds National Day For Truth And Reconciliation

add National Day For Truth And Reconciliation
@stelgenhof stelgenhof added this to the v2.5.0 milestone Aug 30, 2021
@@ -137,7 +138,8 @@ protected function calculateNationalIndigenousPeoplesDay(): void
/**
* Canada Day.
*
* @see https://en.wikipedia.org/wiki/Canada_Day
* @see https://en.wikipedia.org/wiki/Canada_Day and Holidays Act, R.S.C., 1985, c. H-5
Copy link
Member

Choose a reason for hiding this comment

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

If possible, can you add a link to that Holidays Act?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

added in a further commit

@@ -253,4 +258,18 @@ private function calculateLabourDay(): void
$this->locale
));
}

private function calculateNationalDayForTruthAndReconciliation()
Copy link
Member

Choose a reason for hiding this comment

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

Do you have some sources that would provide a description of this holiday?
I looked at this page https://www.statutoryholidays.com/september30-holiday.php and according to that it is not a public holiday.

Copy link
Contributor Author

@ovgray ovgray Aug 30, 2021

Choose a reason for hiding this comment

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

References have been added to the phpdoc for Canada::calculateNationalDayForTruthAndReconciliation.

I do not know what the page to which you refer means by "public holiday" in a Canadian context.

If it means a day on which every employee in Canada is statutorily entitled to a day off with pay, then there are very few of those.

In Canada, constitutional authority to regulate employment is divided between the federal and provincial/territorial governments. Federally regulated workplaces observe federal statutory holidays. All other workplaces observe the statutory holidays of the province or territory in which they are located.
For a holiday to be one on which every employee in Canada is entitled to a day off with pay, it would have to be a statutory holiday federally and in every province/territory. It appears that is only true for New Year's Day, Canada Day, Labour Day and Christmas Day.

The page to which you refer correctly notes that National Day for Truth and Reconciliation is a federal statutory holiday. It is not a statutory holiday in any province/territory. So it is not a statutorily required day off with pay for every employee in Canada.

The Canada provider treats Good Friday, Victoria Day, Thanksgiving Day, Remembrance Day and Boxing Day as holidays. They are all federal statutory holidays. Each is also a statutory holiday in some but not all provinces/territories. Hence, they are not statutorily required days off with pay for every employee in Canada.

Copy link
Member

Choose a reason for hiding this comment

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

Thank you for the extensive information. Public holidays being non-working days or not vary greatly between countries, and as such cause sometimes confusion. For this library I tend to only consider if a holiday is officially recognized by a local government. Irrespective if that means it is a non-working day or not. Because that is often arbitrary and not always regulated by a government.

This is one of the reasons why Yasumi is not perfect in this regards :) I am still looking into that how to do that better in a future release.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

So do you consider this a holiday by your definition?

Copy link
Member

Choose a reason for hiding this comment

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

Yes, for this library any day can be included. Just a matter of the correct classification.

@stelgenhof
Copy link
Member

Thanks for the PR! Can you please update the CHANGELOG.md file reflecting the proposed changes/fixes?

@stelgenhof stelgenhof marked this pull request as draft August 30, 2021 12:31
ovgray added 4 commits August 30, 2021 09:12
add National Day For Truth And Reconciliation
# Conflicts:
#	src/Yasumi/Provider/Canada.php
#	src/Yasumi/data/translations/truthAndReconciliationDay.php
#	tests/Canada/TruthAndReconciliationDayTest.php
add changes to CHANGELOG.md
@ovgray
Copy link
Contributor Author

ovgray commented Aug 30, 2021

changes added to CHANGELOG.md

@ovgray
Copy link
Contributor Author

ovgray commented Aug 31, 2021

Should I squash my commits?

@stelgenhof
Copy link
Member

Should I squash my commits?

You can if you like. I also can do it when merging your PR. Up to you :)

@ovgray
Copy link
Contributor Author

ovgray commented Aug 31, 2021

Might be better if you squash when merging.

@stelgenhof stelgenhof marked this pull request as ready for review September 1, 2021 11:51
@stelgenhof
Copy link
Member

@ovgray BTW Please change this PR to be merged into the develop' branch and not the master`. Thank you!

@ovgray
Copy link
Contributor Author

ovgray commented Sep 1, 2021

I based my PR on the Master branch because I saw that PR#253, which added the Juneteenth holiday to the US provider, was based on and merged into the master branch (and, I note, is not yet in the develop branch).

I will create a new branch based on the develop branch, add my code to it and do a fresh PR.

@ovgray ovgray mentioned this pull request Sep 1, 2021
@stelgenhof
Copy link
Member

@ovgray Ah yes I recall that accidentally I may have merged that Juneteenth feature in the master branch. Anyway, you should be able to change the base branch of a PR without creating a new one: https://docs.github.com/en/github/collaborating-with-pull-requests/proposing-changes-to-your-work-with-pull-requests/changing-the-base-branch-of-a-pull-request

@ovgray
Copy link
Contributor Author

ovgray commented Sep 2, 2021

@stelgenhof
I think the develop branch has too many changes from the master branch for that to work. For one thing, tests in the two branches implement different interfaces. The TruthAndReconciliationDayTest in this PR would have to be edited to be testable in the develop branch, and then would no longer be testable at my end.

I have provided a new PR #257 that makes the same changes to the develop branch, but with a TruthAndReconciliationDayTest that implements the new interface in the develop branch.

I will close this PR in favour of #257.

@ovgray ovgray closed this Sep 2, 2021
@stelgenhof stelgenhof removed this from the v2.5.0 milestone Sep 3, 2021
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