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

Issue #94: Created SG.py (This is a file that handles functionality for the Singapore locale) #108

Open
wants to merge 6 commits into
base: main
Choose a base branch
from

Conversation

nathanielmiller23
Copy link

This issue came from here: #94

What I did:

  • I implemented a file, SG.py to handle the functionality for Singaporean holidays to be used in holidata.
  • For holidays occurring on the same day, I was simply able to use the link referred to by the issue creator: https://www.mom.gov.sg/employment-practices/public-holidays
  • For holidays with variable dates, I hardcoded them, dating back to the year 2011, which is the earliest year for which holidata provides data. The hardcoded dates contain data up until the year 2026 and would likely need to be updated at a later date. From one of my sources:

A majority of the holidays celebrated in Singapore are based on different religious beliefs. Many of these holidays occur on different dates each year because they are based on different calendars and the lunar cycle. The Ministry of Manpower will release the official holiday dates towards the end of each year for the following year.

Christian holidays that use the lunar cycle are based off of the Gregorian calendar. Muslim holidays based off of the lunar cycle in Singapore are calculated by the Majlis Ugama Islam lunar visibility criteria. Hindu holidays are based off of the Hindu lunar calendar. If more than one holiday occurs on the same date because of lunar calculations, an additional holiday is given to replace it and to eliminate “bad luck.”

With this in mind, hard-coding certain holidays for the Singapore locale will likely be a necessity.

Sources:

…Singaporean Holidays. Hardcoded holidays with variable dates up until 2026. Source: publicholidays.sg
…to 2011 for the Singaporean locale. Hardcoded things that have variable dates due to reliance on things like lunar calendars."
Copy link
Member

@lauft lauft left a comment

Choose a reason for hiding this comment

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

@nathanielmiller23 Thanks for your contribution. I left some comments to be addressed before merging. Also, please update the snapshot tests for this new locale.

src/holidata/holidays/SG.py Outdated Show resolved Hide resolved
src/holidata/holidays/SG.py Outdated Show resolved Hide resolved
src/holidata/holidays/SG.py Outdated Show resolved Hide resolved
src/holidata/holidays/SG.py Outdated Show resolved Hide resolved
Copy link
Member

@lauft lauft left a comment

Choose a reason for hiding this comment

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

Thanks for the update. Apart from the comments I made, could you also rebase your branch to the current HEAD?

"""Return the date of Christmas Day for the given year."""
return SG.shift_to_monday_if_weekend(date(year, 12, 25))

# Legal sources:
Copy link
Member

Choose a reason for hiding this comment

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

I apologize for being a bit imprecise with my statement "For all other countries the legal sources are added as a comment".

What I meant was to use a docstring for this information, i.e. wrap in triple quotes and put it to the top.
In general, try to align the layout with the other country files.

@@ -0,0 +1,223 @@
from datetime import date, timedelta
Copy link
Member

Choose a reason for hiding this comment

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

As mentioned in the comment to shift_to_monday_if_weekend, use SmartDayArrow for all dates, i.e. do not use the datetime module at all. Also here, sorry for my imprecision when I just said "Use holidatas class SmartDayArrow..."

See other country files on how to this is done, e.g. the region file for Ceuta where a date map similar to those used by you here is defined, or the country file for Great Britain.

@@ -135,14 +135,6 @@
'region': 'SL',
Copy link
Member

Choose a reason for hiding this comment

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

Those are the wrong test files... 😅 You need to add those for en_SG.

Add your locale to holidata/holidays/init.py, sorted alphabetically into the list, rerun pytest tests/test_holidata.py --update-snapshots to create the snapshots for SG, and add those (and only those). If any other tests are red or have to be modified, that's an error 😉

@lauft
Copy link
Member

lauft commented Aug 17, 2024

@nathanielmiller23 I have dropped the dependency to snapshottest in favor of syrupy; the latter seems better maintained and already supports Python 3.12.

This makes it mandatory to rebase your PR before adding the snapshot files for your locale.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants