-
-
Notifications
You must be signed in to change notification settings - Fork 197
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 Malaysia holiday #17
Conversation
Could you rebase this branch, in order to run the tests here? |
c3bb10d
to
77a7443
Compare
'swk' => $this->regionSarawak($year), | ||
'sgr' => $this->regionSelangor($year), | ||
'trg' => $this->regionTerengganu($year), | ||
default => array_merge( |
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.
By default I wouldn't return any region specific holidays. What you think?
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.
i think it is better to add check if region is set; then throw a new exception invalid region if no match. better flow right? @Nielsvanpach
but the reason i put the default to return all is to allow users to get all holidays in Malaysia.
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.
I'm not aware of the situation in Malaysia, but I think returning all national public holidays by default if no region is given, might be the way to go. This is more in line with implementation for other countries. https://publicholidays.com.my/2024-dates/
37acfb2
to
f0214a1
Compare
'swk' => $this->regionSarawak($year), | ||
'sgr' => $this->regionSelangor($year), | ||
'trg' => $this->regionTerengganu($year), | ||
default => array_merge( |
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.
I'm not aware of the situation in Malaysia, but I think returning all national public holidays by default if no region is given, might be the way to go. This is more in line with implementation for other countries. https://publicholidays.com.my/2024-dates/
I'm probably merging this PR soon, so might be best to use his implementation of the ChineseCalendar. |
029914d
to
0455f04
Compare
0455f04
to
ab27a90
Compare
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.
Perhaps you could take a look at other classes, and refactor the code to make it look more like other Countries?
Examples: Belgium, Albania, Jamaica, ..
7f02190
to
5a28447
Compare
5a28447
to
c929e4b
Compare
Thanks for all the work! |
No description provided.