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

Refactored the rules for calculating holidays in South Korea based on the history of holiday changes. #314

Merged
merged 28 commits into from
Jun 7, 2023

Conversation

barami
Copy link
Contributor

@barami barami commented May 7, 2023

@stelgenhof
Hi there. It's been a while.

Recently, the South Korean government passed a bill designating Christmas and Buddha's Day as alternative holidays.

In response, we modified the code for calculating alternative holidays and refactored the scattered and hard-to-understand structure into one that follows the changes over the years.

Here are the changes we made

  1. Updated the KASI documentation reference link in the SouthKorea Provider note and added a link to the conversion utility.
  2. Added alternative holidays (Buddha's Day, Christmas).
  3. Added a generator method to create an instance of a holiday, such as the CommonHolidays trait.
  4. Collected the names of each holiday in one place and organized them into a HOLIDAY_NAMES constant.
  5. Fixed an incorrect date for Buddha's Birthday in 2001.
  6. Fixed an incorrect year for Chuseok in 1995.
  7. Fixed the years that Armed Forces Day is included as a public holiday
  8. Added the 1959, 1960, and 1989 cases to the 'calculateOldSubstituteHolidays' method.
  9. Fixed overall unit tests for the SouthKorea provider.
  10. Adding unit tests for United Nations Day.

Best regards.

@stelgenhof
Copy link
Member

@barami Thanks for this PR. Are all your changes done? I will then review it. There are some pipeline errors but these are not related to your changes.

@barami
Copy link
Contributor Author

barami commented May 12, 2023

@barami Thanks for this PR. Are all your changes done? I will then review it. There are some pipeline errors but these are not related to your changes.

@stelgenhof
Sorry, I've been a little busy lately and haven't gotten around to changing the unit tests.
I'll change the unit test, commit it, and let you know in the comments.

@barami barami force-pushed the develop branch 4 times, most recently from b3db365 to 6a5e5c1 Compare May 14, 2023 14:32
@barami
Copy link
Contributor Author

barami commented May 14, 2023

@stelgenhof

Thank you for your wait.
I've made all the fixes and will write another PR if there are any other fixes in the future.

@stelgenhof
Copy link
Member

@barami Sorry I missed your last message. I'll have a look this weekend. Thanks for all the work!

Copy link
Member

@stelgenhof stelgenhof left a comment

Choose a reason for hiding this comment

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

Can you update the CHANGELOG file as well? Thanks!

@barami
Copy link
Contributor Author

barami commented May 22, 2023

Can you update the CHANGELOG file as well? Thanks!

Thanks for your review. I've updated the CHANGELOG file.

barami added 16 commits May 25, 2023 22:28
…h Korea celebrated Buddha's Birthday on May 1 in 2001.
… methods to use year parameter instead of instance member.
@barami
Copy link
Contributor Author

barami commented May 25, 2023

There is a conflict with a recent change in the upstream commit, so we rebase and request again.
Thanks.

@barami barami reopened this May 25, 2023
@stelgenhof stelgenhof merged commit 002ce9a into azuyalabs:develop Jun 7, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants