-
-
Notifications
You must be signed in to change notification settings - Fork 156
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
Adds UK bank holiday for Queen Elizabeth II’s funeral #287
Adds UK bank holiday for Queen Elizabeth II’s funeral #287
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@freshleafmedia Thank you very much for this PR! Looks good to me, except it is missing a test assertion for this bank holiday with the United Kingdom provider (see testBankHolidays
).
Is this bank holiday observed in the related countries of England, Wales, Scotland and Northern Ireland as well? (I assume they do, but just want to make sure). If so, then the same unit tests need to be added for those countries as well.
@stelgenhof I'm not sure adding it to the It is observed in England, Wales, Scotland and Northern Ireland. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@freshleafmedia The unit test for this bank holiday will just assert if it is correct, however the providers have tests that assert the list of bank holidays for a given year. Since this is a new bank holiday, it should be added to that test/assertion.
@stelgenhof I think I understand. Does that mean that the public function testBankHolidays(): void
{
$this->assertDefinedHolidays([
'newYearsDay',
'easterMonday',
'mayDayBankHoliday',
'springBankHoliday',
'secondChristmasDay',
], self::REGION, $this->year, Holiday::TYPE_BANK);
$this->assertDefinedHolidays([
'queenElizabethFuneralBankHoliday',
], self::REGION, 2022, Holiday::TYPE_BANK);
} |
@freshleafmedia Yes, that should do it. Another way would be to build the list of holidays dynamically: public function testOfficialHolidays(): void
{
$holidays = [
'newYearsDay',
'easterMonday',
'internationalWorkersDay',
'ascensionDay',
'pentecostMonday',
'nationalDay',
'assumptionOfMary',
'allSaintsDay',
'christmasDay',
'secondChristmasDay',
];
$year = $this->generateRandomYear();
if ($year >= Luxembourg::EUROPE_DAY_START_YEAR) {
$holidays[] = 'europeDay';
}
$this->assertDefinedHolidays($holidays, self::REGION, $year, Holiday::TYPE_OFFICIAL);
} (Taken from the Luxembourg provider test). |
Hello, would be cool if this PR was merged/released as it would be a bit late otherwise 😛 |
Approved the PR as the bank holiday is occurring today. Will add the remaining unit test afterwards. |
No description provided.