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

fix mid autumn & national day #413 #414

Merged
merged 10 commits into from
Oct 19, 2024

Conversation

ruralscenery
Copy link

@ruralscenery ruralscenery commented Sep 18, 2024

References (non-English):

Starting from 2024, the "anniversaries and holidays that should be taken off" will be adjusted to "the day before New Year's Eve" and "Children's Day and National Tomb Sweeping Day"

@maread99 maread99 self-assigned this Sep 20, 2024
@maread99 maread99 added the calendar update Calendar needs updating label Sep 20, 2024
Copy link
Collaborator

@maread99 maread99 left a comment

Choose a reason for hiding this comment

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

Hi @ruralscenery, thank you for this PR. Seems that it's failing the tests on a simple flake 8 linting issue:

./exchange_calendars/exchange_calendar_xtai.py:147:1: W293 blank line contains whitespace

The holiday changes seem to be significant. I've included comments in the review asking for:

  • references for those holidays where extra Mondays and Fridays are no longer being observed post 2023.
  • dates to be added to the test fixture 'non_holidays_sample' to test that for these holidays the extra days are not being evaluated as holidays post 2023.

Thanks!

exchange_calendars/exchange_calendar_xtai.py Outdated Show resolved Hide resolved
exchange_calendars/exchange_calendar_xtai.py Outdated Show resolved Hide resolved
exchange_calendars/exchange_calendar_xtai.py Outdated Show resolved Hide resolved
exchange_calendars/exchange_calendar_xtai.py Outdated Show resolved Hide resolved
exchange_calendars/exchange_calendar_xtai.py Outdated Show resolved Hide resolved
exchange_calendars/exchange_calendar_xtai.py Outdated Show resolved Hide resolved
exchange_calendars/exchange_calendar_xtai.py Outdated Show resolved Hide resolved
exchange_calendars/exchange_calendar_xtai.py Outdated Show resolved Hide resolved
exchange_calendars/exchange_calendar_xtai.py Show resolved Hide resolved
tests/test_xtai_calendar.py Show resolved Hide resolved
https://www.dgpa.gov.tw/information?uid=82&pid=11398 Section 2
https://news.pts.org.tw/article/638479

Starting from 2024, the "anniversaries and holidays that should be taken off" will be adjusted to "the day before New Year's Eve" and "Children's Day and National Tomb Sweeping Day";
@ruralscenery ruralscenery requested a review from maread99 October 14, 2024 07:46
@maread99
Copy link
Collaborator

Hi @ruralscenery, thanks for making the changes. Sorry for not having looked at this earlier - I will do before the end of the week.

At the moment the tests are failing on the following linting issues:

./exchange_calendars/exchange_calendar_xtai.py:152:20: W291 trailing whitespace
./exchange_calendars/exchange_calendar_xtai.py:160:1: W293 blank line contains whitespace
./exchange_calendars/exchange_calendar_xtai.py:170:20: W291 trailing whitespace
./exchange_calendars/exchange_calendar_xtai.py:187:12: W291 trailing whitespace

@maread99
Copy link
Collaborator

Hi @ruralscenery

There are a lot of changes to the holidays here. I'm unable to find an English reference to verify the changes. That's not a problem, but I would ask that you confirm a couple of changes that would result from this PR are indeed intended and correct rather than unintentional. I've noted them in the latest review comments (the no longer making up for 'dragon_boat_festival' and 'mid_autumn_festival' holidays that fall on the weekend).

@ruralscenery
Copy link
Author

Appreciate your attention to detail regarding the Dragon Boat Festival and Mid-Autumn Festival falling on weekends.

I can confirm that the changes are made in accordance with "Anniversaries and holidays are implemented in accordance with the provisions of these regulations"

According to the provisions of the new system, in the next five years, except for 2026, there will be no adjustment to holidays and make-up work and classes. The rest of the years will be adjusted once, and they all happen to be on the day before New Year's Eve.

non_holidays_sample: 2024-09-16, 2024-10-11, 2030-09-13

Revises documentation and renames some functions on
`exchange_calendar_xtai`.

Also simplifies `weekend_makeup` (renamed as
`nearest_weekday_from_2014`).
@maread99
Copy link
Collaborator

Thank you for that confirmation @ruralscenery.

You'll see I've added a commit which revises some documentation and renames some functions. The only actual code it touches is the function that was weekend_makeup (renamed as nearest_weekday_from_2014) which I've simplified. The changes are intended to make it all a bit clearer and are more concerned with was there already than your PR. The commit also formats to black.

Either let me know that you're happy with what I've done or push any proposed changes. Then I'll merge it and cut a new release so that all this is reflected in the latest PyPI version.

Thanks again for the PR!

@ruralscenery
Copy link
Author

Thank you for the effort of review, It's look good to me

@maread99 maread99 merged commit 032a556 into gerrymanoim:master Oct 19, 2024
7 checks passed
brian-from-quantrocket pushed a commit to quantrocket-llc/exchange_calendars that referenced this pull request Nov 22, 2024
Updates XTAI holidays from 2024 (including mid autumn and national day).

Also:
- includes holidays for KRATHON typhoon (02 Oct 24 - 03 Oct 24).
- renames xtai module functions and revises documentation for clarify.

---------

Co-authored-by: Marcus Read <marcusaread@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
calendar update Calendar needs updating
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants