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 Jordan holidays #209

Closed
wants to merge 7 commits into from
Closed

Conversation

yousefdev20
Copy link

@yousefdev20 yousefdev20 commented Feb 24, 2024

Contributing a new country ?

  • Have you checked to ensure there aren't other open Pull Requests for the same country?
  1. Create a new class in the Countries directory. It should extend the Country class.
  2. Add a test for the new country in the tests directory.
  3. Run the tests so a snapshot gets created.
  4. Verify the result in the newly created snapshot is correct.

@yousefdev20 yousefdev20 changed the title Jordanian holidays Add Jordan holidays Feb 24, 2024

return [

'jo' => [
Copy link
Member

Choose a reason for hiding this comment

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

Can you use class consts for this? Have a look at Egypt or Bahrain

Copy link
Author

Choose a reason for hiding this comment

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

@Nielsvanpach
Why I used this approach:

  1. It's hard to read around 200 lines of code to read the elements.
  2. in some countries the holidays are shared or in the same region.
  3. at the end the Islamic directory is loadable.

Copy link
Member

Choose a reason for hiding this comment

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

2 and 3 seem valid points.

Although, using jo as a key doesn't feel like the way to go for being shareable across countries?

Also, if you have a look at Turkey, I prefer that format. What the benefit of using the extra date key?

Also, for multiday holidays it's possible to have it twice in a single year:

1999 => '01-19',
2000 => [
    '01-08',
     '12-27',
],

Copy link
Author

Choose a reason for hiding this comment

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

jo it may change,

date is more readable,

The third note is valid.


use Carbon\CarbonImmutable;

trait IslamicHolidays
Copy link
Member

Choose a reason for hiding this comment

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

We already have an IslamicCalendar trait which you can use

Copy link
Author

@yousefdev20 yousefdev20 Mar 22, 2024

Choose a reason for hiding this comment

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

This trait depends on the constants variable and I explained the issue in the above replay.

use Carbon\CarbonPeriod;
use Carbon\CarbonImmutable;

trait WeekendHolidays
Copy link
Member

Choose a reason for hiding this comment

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

I think the Observable trait is what you need

@spatie-bot
Copy link

Dear contributor,

because this pull request seems to be inactive for quite some time now, I've automatically closed it. If you feel this pull request deserves some attention from my human colleagues feel free to reopen it.

@spatie-bot spatie-bot closed this Jul 23, 2024
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