-
Notifications
You must be signed in to change notification settings - Fork 473
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
Improve Thailand holidays #929
Conversation
Great work! |
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.
Here is 2 critical changes (special_holidays and redundant parenthesis in Wisakha Bucha code)
034f734
to
9a764a9
Compare
Co-authored-by: ~Jhellico <KJhellico@users.noreply.github.com>
Co-authored-by: ~Jhellico <KJhellico@users.noreply.github.com>
Co-Authored-By: ~Jhellico <KJhellico@users.noreply.github.com>
Co-Authored-By: ~Jhellico <KJhellico@users.noreply.github.com>
@KJhellico Thank you so much for the help :) I've fixed all the mentioned errors and applied all other suggestions; feel free to let me know if you need anything else. |
PR should be into |
@KJhellico The pre-commit checks from I'm willing to update this pull request so that the new changes would work perfectly with the i18n PoC (#827); just let me know if you plan to merge that change in first. My other concern is the fact that |
Co-authored-by: ~Jhellico <KJhellico@users.noreply.github.com>
Added tests for Thailand
Co-Authored-By: ~Jhellico <KJhellico@users.noreply.github.com>
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.
Seems good
- Simplified __init__ section for Thailand's, removed for _ThaiLuniSolar - Simplified _ThaiLuniSolar holiday codes - _ThaiLuniSolar's comment are now docstrings - Move all Thailand's holiday references to above limitations' section - Remove Thailand's duplicate date checks (with TODO comments instead) - Thailand's In Line comments are also moved to each holiday's header for better readability Co-Authored-By: Arkadii Yakovets <2201626+arkid15r@users.noreply.github.com>
Co-Authored-By: Arkadii Yakovets <2201626+arkid15r@users.noreply.github.com>
Co-authored-by: ~Jhellico <KJhellico@users.noreply.github.com>
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.
Looks good to me except a few minor issues.
Thanks for addressing all the comments!
Co-Authored-By: Arkadii Yakovets <2201626+arkid15r@users.noreply.github.com>
We failed coverall tests as the two extra cases added (Atthami Bucha, End of Buddhist Lent) got no active use cases. Should we just comment them out for now or add tests for them somewhere? |
If you're comfortable with adding those - it would be just perfect! Otherwise I'd take a look into it early next week. I'm going to merge this PR later today. This PR has taken a huge amount of work. Thank you @PPsyrius, @KJhellico! |
I'll leave the issue of test cases for those two dates in your hands then - from a quick check, while TYSM for all the help :) |
A subclass of :py:class:
HolidayBase
representing public holidays in Thailand.(Based on South Korean and Singaporean Implementation)
Limitations:
References: