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

Add holidays for South Africa #30

Merged
merged 1 commit into from
Jan 23, 2024
Merged

Conversation

pierredup
Copy link
Contributor

No description provided.

@pierredup pierredup force-pushed the south-africa branch 2 times, most recently from 23ec0b1 to 543050a Compare January 18, 2024 08:42
@Nielsvanpach
Copy link
Member

Thanks! Could you rebase on main, that way I can run the tests here

Copy link
Member

@Nielsvanpach Nielsvanpach left a comment

Choose a reason for hiding this comment

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

Thanks! Could you fix the failing tests on CI? Probably using $this->easter() will fix it.

src/Countries/SouthAfrica.php Outdated Show resolved Hide resolved
tests/Countries/SouthAfricaTest.php Outdated Show resolved Hide resolved
@Nielsvanpach
Copy link
Member

There are 2 South Africa PRs, but with different results it seems. Could you both verify which one is correct? #32

@pierredup
Copy link
Contributor Author

Both PRs looks to implement the same days. This one handles an additional edge case, where if a holiday rolls over from the Sunday to the Monday, it shouldn't be added if the Monday is already a Holiday (E.G if 25 Dec falls on a Sunday, it can't roll over to the Monday since the 26 Dec (the following Monday) will already be a holiday).

@johanmeiring
Copy link

Ah yes, the edge case that is covered here is indeed correct. This is the more complete implementation, and should be the one that gets merged. I'll close my PR :-)

P.S. I'll try to remember to open a PR once we know what our general election date will be, as that is very likely to be proclaimed a public holiday as well.

@Nielsvanpach
Copy link
Member

Thanks!

@Nielsvanpach Nielsvanpach merged commit a5b467a into spatie:main Jan 23, 2024
8 checks passed
@pierredup pierredup deleted the south-africa branch January 23, 2024 13:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants