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 Sri Lanka holidays #2228

Open
wants to merge 25 commits into
base: dev
Choose a base branch
from
Open

Conversation

Prateekshit73
Copy link
Contributor

Proposed change

Added Sri Lanka holidays
Closes #1272

Type of change

  • New country/market holidays support (thank you!)
  • Supported country/market holidays update (calendar discrepancy fix, localization)
  • Existing code/documentation/test/process quality improvement (best practice, cleanup, refactoring, optimization)
  • Dependency update (version deprecation/pin/upgrade)
  • Bugfix (non-breaking change which fixes an issue)
  • Breaking change (a code change causing existing functionality to break)
  • New feature (new holidays functionality in general)

Checklist

Copy link

codecov bot commented Jan 14, 2025

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 100.00%. Comparing base (7566884) to head (0d575ae).

Additional details and impacted files
@@            Coverage Diff            @@
##               dev     #2228   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files          196       197    +1     
  Lines        11852     11918   +66     
  Branches      1712      1714    +2     
=========================================
+ Hits         11852     11918   +66     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Makefile Outdated Show resolved Hide resolved
This reverts commit 8b59449.
Copy link
Collaborator

@PPsyrius PPsyrius left a comment

Choose a reason for hiding this comment

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

Nice to see new country being added 🚀

Makefile Outdated Show resolved Hide resolved
holidays/locale/en_US/LC_MESSAGES/LK.po Outdated Show resolved Hide resolved
holidays/locale/ta_LK/LC_MESSAGES/LK.po Outdated Show resolved Hide resolved
holidays/locale/si_LK/LC_MESSAGES/LK.po Outdated Show resolved Hide resolved
holidays/countries/sri_lanka.py Outdated Show resolved Hide resolved
holidays/countries/sri_lanka.py Outdated Show resolved Hide resolved
@PPsyrius
Copy link
Collaborator

I'm working up a solution for Poya Day calculations at the moment and will create a suggestion PR with them later - I was originally planned to base them on Thai LuniSolar calendar ones but it appears the results aren't a 100% match yet

@Prateekshit73
Copy link
Contributor Author

I'm working up a solution for Poya Day calculations at the moment and will create a suggestion PR with them later - I was originally planned to base them on Thai LuniSolar calendar ones but it appears the results aren't a 100% match yet

If there's anything I can do to assist you with the Poya Day calculations, please let me know. I’d be more than happy to help out in any way I can!

@arkid15r
Copy link
Collaborator

@PPsyrius anything we should wait from you or this is good for review?

@PPsyrius
Copy link
Collaborator

@arkid15r no, many poya days assignments are still incorrect at the moment

@Prateekshit73
Copy link
Contributor Author

Thanks for the update, @PPsyrius! If there's anything specific you'd like me to look into, just let me know. I'm happy to contribute!

@KJhellico KJhellico changed the title Added Sri Lanka holidays Add Sri Lanka holidays Jan 23, 2025
@PPsyrius
Copy link
Collaborator

PPsyrius commented Jan 24, 2025

@Prateekshit73 I've created a PR to address Poya day assignment and some other issues I've come across while working on them. Sinhala Lunar calendar method of inserting an extra lunar month every 33-month cycle makes it impractical to compute so I've ended up hardcoding 2023-2025 ones for now.

  • Implements SinhalaCalendarHolidays (for all Poya holidays since none of these matches the Thai nor Buddhist (East Asian Mahayana) calculation method.
  • Adds SriLankaHinduHolidays for exact Deepavali/Divali entries (again, Sri Lankan ones sadly didn't match India's).
  • Switch en_US, sk_LK, and tl_LK localization to the ones used in the official 2025 calendar.
  • 100% exact match for 2003-2025 calendar years as seen from [1] [2] [3] .

I think I might have missed a test case somewhere, but the results from local tests sadly aren't that descriptive so I'm leaving that for you to sort out later 🙏

@Prateekshit73
Copy link
Contributor Author

I feel that instead of helping, I often end up increasing your workload. Hopefully, I will complete the allotted work correctly this time. Thank you for your help, @PPsyrius sir.

@PPsyrius
Copy link
Collaborator

PPsyrius commented Jan 24, 2025

@Prateekshit73 Nah, it's fine - it's just that I've vastly underestimated Sri Lankan holiday complexity myself too.

I heard they're Theravada like the Indochinese trios (🇹🇭🇱🇦🇰🇭) so I thought their implementation would be quite similar myself, until a quick test with real 2022 data ended in failure - and it just went down the rabbit hole from there... 🥲

This is probably on par with the unimplemented Myanmar ones that I kept in the backburner.

Anyway, if you're still up for this, there's a list of African/Caribbean/Pacific here that's likely more straightforward for new contributors as they're mostly Gregorian-based i.e. Saint Lucia and Fiji.

@Prateekshit73
Copy link
Contributor Author

The output of make coverage suggests the following:

holidays\countries\sri_lanka.py                                 84      0      8      1    99%   174->179

This corresponds to the following code snippet:

        if maha_sivarathri_date:
            # Maha Sivarathri Day.
            self._add_holiday(tr("මහ සිවරාත්රි දිනය"), maha_sivarathri_date)

        # Deepavali was considered a workday in 2003.
        if self._year >= 2004:

I attempted to implement a test for this with the following code:

def test_maha_sivarathri(self):
  self.assertHolidayName(
      "මහ සිවරාත්රි දිනය",
      "2003-03-01",
      ...
  )

However, I am still getting a coverage of 99%.

@PPsyrius
Copy link
Collaborator

@arkid15r @KJhellico quick sidenote: CodeCov on PC isn't showing for the new commits and I don't even see their test message anywhere at all on the mobile version of the GitHub app.

SonarCloud does support code coverage display too, so we might want to migrate to that tool later for a simpler review process in the future.

@KJhellico
Copy link
Collaborator

holidays\countries\sri_lanka.py                                 84      0      8      1    99%   174->179

For full coverage it's required to execute a branch where maha_sivarathri_date is None. But there is no such case in years range 2003-2025 (defined by start_year/end_year values for LK).

@KJhellico
Copy link
Collaborator

@arkid15r @KJhellico quick sidenote: CodeCov on PC isn't showing for the new commits and I don't even see their test message anywhere at all on the mobile version of the GitHub app.

As I understand, it's because on latest commits the tests fail and coverage report not being uploaded.

@Prateekshit73
Copy link
Contributor Author

For full coverage it's required to execute a branch where maha_sivarathri_date is None. But there is no such case in years range 2003-2025 (defined by start_year/end_year values for LK).

Hi Sir @KJhellico,

Thank you for your insights! Given that maha_sivarathri_date is never None for the years 2003-2025, do you think it would be feasible to change the start_year to a year before 2003? This could help us create a test case that triggers the branch where maha_sivarathri_date is None, allowing us to achieve full coverage.

As a referenced in the code itself, Sri Lanka's Holidays Act (No. 29 of 1971) was first proclaimed on September 2nd, 1971, which might provide context for setting the start_year.

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.

Add Sri Lanka holidays
4 participants