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 - Pakistan #19

Merged
merged 5 commits into from
Jun 13, 2024
Merged

Conversation

muhammad-nauman
Copy link
Contributor

@muhammad-nauman muhammad-nauman commented Jan 17, 2024

Overview:

This pull request adds the country class for Pakistan. It lists the fixed federal holidays from the list here.

All the relevant test files are also added.

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 rebase this branch, in order to run the tests here?

src/Countries/Pakistan.php Outdated Show resolved Hide resolved
@muhammad-nauman
Copy link
Contributor Author

@Nielsvanpach I do not understand the reason behind the failing action, can you help me with that?

/** @return array<string, CarbonImmutable> */
protected function variableHolidays(int $year): array
{
// The variable holidays are all follow lunar calendar so their dates are not confirmed.
Copy link
Member

Choose a reason for hiding this comment

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I checked the implementation, the solution provides the dates but there is no surety that the calendar will follow thsoe exact dates. Should I go for it anyway?

Copy link
Member

Choose a reason for hiding this comment

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

I wouldn't copy-paste the Turkey solution. But a similar approach can be done for Pakistan?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@Nielsvanpach I tried adapting the solution. The problem is, that we have 4 different sets of holidays that follow the lunar calendar(total of 10 holidays) and it is making the code a lot more complex and even after that, the dates will not be accurate.

tests/Countries/PakistanTest.php Outdated Show resolved Hide resolved
Co-authored-by: Niels Vanpachtenbeke <[email protected]>
@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 Jun 13, 2024
@Nielsvanpach Nielsvanpach reopened this Jun 13, 2024
@Nielsvanpach Nielsvanpach merged commit 7b15433 into spatie:main Jun 13, 2024
10 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.

None yet

3 participants