search

azuyalabs / yasumi

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

Add substituted holidays for Australia #201

Closed c960657 closed 4 years ago

c960657 commented 4 years ago

Fixes #199.

I don't know anything about Australian holidays, but perhaps @jbroudou can verify this?

Basically we now mark both the original holiday and the substitute holiday as TYPE_OFFICIAL holidays. This applies to all holidays which are moved to the following Monday.

jbroudou commented 4 years ago

Hi,

Is this applied to all Australian holidays?

Not all holidays in Australia have the condition where if the public holiday falls on a Sunday, then both the Sunday and Monday are holidays. It depends on the holiday and state.

Ideally you would set the BOTH condition (i.e. Sunday and Monday) on by default for all holidays, but then have a flag where you can switch it off so that the Monday is the only holiday.

c960657 commented 4 years ago

This PR does not change which holidays are moved to Mondays. Whether each holiday should be moved is specified by two boolean arguments for calculateHoliday(..., $moveFromSaturday, $moveFromSunday, ...).

What this PR does is that if a holiday is moved from a Saturday or Sunday to a Monday, the original holiday on Saturday/Sunday will be marked in addition to the holiday on Monday. The two days will be labeled e.g. “Australia Day” and “Australia Day observed”, respectively.

stelgenhof commented 4 years ago

In addition to @c960657 comment, just wanted to confirm that the 'business' rules whether a holiday is being substituted lies already with each individual holiday. In some other countries, holiday substitution applies to all holidays so a more generic logic/code is applied.

stelgenhof commented 4 years ago

@c960657 Would you mind having a look at my comments? Thanks!

c960657 commented 4 years ago

Sure, I'll take a look next week.

github-actions[bot] commented 4 years ago

This pull request has been open 60 days with no activity. Please remove the stale label or comment, or this will be closed in 10 days.

c960657 commented 4 years ago

Sorry for not responding to this sooner.

I have added an assertion on the holiday type for the subsituted holidays.

It is true that calculateHoliday() is used elsewhere, but only Australia Day uses the substitute holiday logic. Either it is called with false, false, meaning that no substitution should happen, or it is called with holidays that always fall on e.g. a Tuesday. This is why no other tests were failing after my changes. So I went ahead and replaced all these calls to calculateHoliday() with addHoliday().

Also, I removed a bunch of unused asserts. If we just asserted that $foo is an instance of Holiday, we don't need to assert that it is not null.

stelgenhof commented 4 years ago

@c960657 It's looking good, however can you again look at my comments? There could be an additional check for the SubstituteHoliday instance in the tests.

c960657 commented 4 years ago

I added assertSubstituteHoliday(). This asserts that the holiday is the right subclass. Doesn't this address your comment, or did you mean something else?

stelgenhof commented 4 years ago

@c960657 Yes, thank you! I missed that the code was changed :) There are now some conflicts. Could you check these? Then I will merge the PR.

c960657 commented 4 years ago

Done :)