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 Canada provider #215

Merged
merged 14 commits into from
May 5, 2020
Merged

Added Canada provider #215

merged 14 commits into from
May 5, 2020

Conversation

lux
Copy link
Contributor

@lux lux commented May 2, 2020

No description provided.

@stelgenhof
Copy link
Member

Very nice! Thank you very much for this PR @lux Much appreciated as there were a few requests for Canada :)
I'll have a look soon and merge it if all looks fine.

Cheers! Sacha

@c960657
Copy link
Contributor

c960657 commented May 3, 2020

This is great work!

A few comments:

Each provider should use a timezone relevant for the specific province. For national provider, I think it would be natural to use the timezone for the capital, Ottawa.

Though not implemented throughout Yasumi, it is my personal opinion that translations for holidays implemented in more than one provider are better stored in translation files in src/Yasumi/data/translations. This makes them easier to maintain.

@stelgenhof stelgenhof added this to the v2.3.0 milestone May 3, 2020
@lux
Copy link
Contributor Author

lux commented May 3, 2020

I've updated all the timezones for Canada and each province/territory. There wasn't a timezone specific to Ottawa, but "America/Toronto" is the same timezone as Ottawa so I went with that.

I wasn't sure how best to approach extracting translations since I don't see other countries in src/Yasumi/data/translations.

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.

Just recently there is a change in the way the DateTimeZone class is used in the code. Using the DateTimeZoneFactory class helps improve the performance of the library. Please change the following (example):

$newyearsday = new DateTime("$this->year-01-01", new DateTimeZone($this->timezone));
to

$newyearsday = new DateTime("$this->year-01-01", DateTimeZoneFactory::getDateTimeZone($this->timezone));

src/Yasumi/Provider/Canada/NewfoundlandAndLabrador.php Outdated Show resolved Hide resolved
src/Yasumi/Provider/Canada/Quebec.php Outdated Show resolved Hide resolved
src/Yasumi/Provider/Canada/NorthwestTerritories.php Outdated Show resolved Hide resolved
@c960657
Copy link
Contributor

c960657 commented May 4, 2020

I wasn't sure how best to approach extracting translations since I don't see other countries in src/Yasumi/data/translations.

The translations are not grouped by country but by holiday. E.g. Labour Day is already defined in src/Yasumi/data/translations/labourDay.php and already has an English translation. So you can just remove the inline translations from the PHP class and add the French translation to labourDay.php:

         $this->addHoliday(new Holiday(
             'labourDay',
-            ['en' => 'Labour Day', 'fr' => 'Fête du Travail'],
+            [],
             new DateTime("first monday of september $this->year", DateTimeZoneFactory::getDateTimeZone($this->timezone)),

For translations not already present, just create a new file with the day's short/internal name. The file will be loaded automatically.

If a day has a different name in Canadian French than in “general” French, you can use the key fr_CA.

@stelgenhof
Copy link
Member

@lux Thanks for making the latest changes! Looking a lot better now.

One thing I noticed is that you have a lot of duplicate logic. For example VictoriaDay, Thanksgiving have the same code in the various Canadian states. You can simplify this by extracting these into a common function and move these to the Canada class for example (or even a separate class if you prefer).

This way you can reuse the holiday logic in the various states (unless the logic is different of course).

Cheers! Sacha

@lux
Copy link
Contributor Author

lux commented May 4, 2020

@c960657 that makes sense. I'll look at changing that up.

@stelgenhof I ended up duplicating those because of inconsistencies across Canada. Victoria Day, for example, is celebrated in 11 of 13 provinces/territories, called National Patriot's Day in Quebec, and not recognized in Newfoundland. Since provinces inherit from the country provider, I could move the method definition up a level but leave it to the provinces to call addHoliday() if that works better. Similarly, Thanksgiving is only recognized in 9 of 13.

@stelgenhof
Copy link
Member

@lux Makes sense, however I would still recommend to make a common method for those states that share that common logic and make exceptions where needed. That would save time in (future) debugging and testing :)

@lux
Copy link
Contributor Author

lux commented May 4, 2020

The shared holidays are only defined in the Canada provider now, and I've moved the main Canada holidays into translations but didn't have time to do the translations for the province-specific names yet.

@stelgenhof
Copy link
Member

Thanks! It's looking good. Take your time :)

@stelgenhof stelgenhof merged commit 91a2859 into azuyalabs:develop May 5, 2020
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.

3 participants