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 Egyptian holidays #155

Merged
merged 21 commits into from
Feb 7, 2024
Merged

Conversation

wessama
Copy link
Contributor

@wessama wessama commented Jan 21, 2024

Public holidays in Egypt, both observed and national holidays, including checks for whether or not a holiday falls on a weekday/weekend and when it should be observed. This is synced with the way Google Calendar does it.

As noted in #56 and #90, the use of geniusts/hijri-dates is necessary to (pretty much) accurately calculate when Islamic holidays will be observed during a given year. When they're observed differs from one country to another, even if they're all celebrated concurrently.

All tests pass.

@wessama wessama force-pushed the wessama/add-egypt-holidays branch from ec4826d to 5e5fc8e Compare January 22, 2024 09:30
@wessama wessama force-pushed the wessama/add-egypt-holidays branch from 6a33c92 to 005220e Compare January 22, 2024 14:54
@stevebauman
Copy link

stevebauman commented Jan 22, 2024

Underlying package is abandoned and also doesn't have any tests -- would be very skeptical to depend on it.

It is also restricted to Carbon v1 & v2, which is preparing to have a new v3 release soon, so this package will be incompatible with Laravel 11: laravel/framework#49764

@wessama
Copy link
Contributor Author

wessama commented Jan 22, 2024

Underlying package is abandoned and also doesn't have any tests -- would be very skeptical to depend on it.

It is also restricted to Carbon v1 & v2, which is preparing to have a new v3 release soon, so this package will be incompatible with Laravel 11: laravel/framework#49764

Thanks. I'm working together with a few other PR authors to come up with a way to centralize this logic in the Country class.

@wessama
Copy link
Contributor Author

wessama commented Feb 3, 2024

@Nielsvanpach I understand your hesitance to rely on an outdated package (one without tests, as well). So I adopted the Turkey approach in #70 and added those holidays statically. But I couldn't find a source that has been reliably tracking these holidays in Egypt prior to 2005. I double checked with Google Calendar and a few other sources, but the information on Wikipedia is incorrect.

You might know by now that there's no way to accurately predict when these holidays will be observed because of the variance involved, but this provides as good an approach as any (to my knowledge, anyway. I'd welcome anyone to correct this). Maybe this can be maintained in the future to add any drastic changes.

But apart from Islamic holidays, everything else can be precisely calculated.

I also found conflicts on the main branch with the Egypt class, which previously didn't account for Islamic holidays at all (and those actually factor into the fiscal year here and shouldn't be ignored, not by any reliable source of information anyway. I'd hate to be a business relying on this package only to find out it doesn't match real calendars). We've also been provided days off in lieu of national holidays that fall on a weekday or a weekend (which is Friday and Saturday here) for the past 3-4 years, this was also absent in the existing Egypt class. Variable holidays like the "Spring Festival" were also hardcoded even though this method in the Country class can be used.

protected function orthodoxEaster(int $year): CarbonImmutable

Other than that, I found the existing lang translations a bit too Google translate-ish so I replaced them with more sensible, true-to-life translations and added missing ones:

...and used the Egyptian Presidency's calendar for reference.

I kept the original snapshot to keep myself honest. All tests still pass.

@Nielsvanpach
Copy link
Member

Thanks!

@Nielsvanpach Nielsvanpach merged commit 9df73f8 into spatie:main Feb 7, 2024
7 of 8 checks passed
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