azuyalabs / yasumi

The easy PHP Library for calculating holidays
https://www.yasumi.dev
Other
1.04k stars 152 forks source link

Added Canada provider #215

Closed lux closed 4 years ago

stelgenhof commented 4 years ago

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 commented 4 years ago

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.

lux commented 4 years ago

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.

c960657 commented 4 years ago

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 commented 4 years ago

@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 commented 4 years ago

@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 commented 4 years ago

@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 commented 4 years ago

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 commented 4 years ago

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