Skip to content

Add Singapore Exchange (SGX) holidays#3719

Open
pareshjoshij wants to merge 4 commits into
vacanza:devfrom
pareshjoshij:add_SGX
Open

Add Singapore Exchange (SGX) holidays#3719
pareshjoshij wants to merge 4 commits into
vacanza:devfrom
pareshjoshij:add_SGX

Conversation

@pareshjoshij

Copy link
Copy Markdown
Contributor

This PR adds the holidays for the Singapore Exchange (SGX) financial calendar and l10n support.
Part of GSoC (#3424)

  • New country/market holidays support (thank you!)
  • Supported country/market holidays update (calendar discrepancy fix, localization)
  • Existing code quality improvement (best practice, cleanup, refactoring, optimization)
  • Documentation update
  • Test suite update
  • Development process update (CI/CD, project configuration, internal tooling)
  • 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

Copilot AI review requested due to automatic review settings July 25, 2026 15:12

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.

@coderabbitai

coderabbitai Bot commented Jul 25, 2026

Copy link
Copy Markdown
Contributor

Review Change Stack

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: ASSERTIVE

Plan: Pro Plus

Run ID: 942b7fd9-43f5-40c4-9197-ade6c051f139

📥 Commits

Reviewing files that changed from the base of the PR and between c0743b5 and 329a4ec.

📒 Files selected for processing (3)
  • holidays/financial/singapore_exchange.py
  • holidays/locale/th/LC_MESSAGES/XSES.po
  • tests/financial/test_singapore_exchange.py

Summary by CodeRabbit

  • New Features

    • Added Singapore Exchange (SGX) holiday calendar support, including full-day market holidays and half-day closures for Christmas Eve, New Year’s Eve, and Chinese New Year’s Eve (market closes at 12:00 p.m. SGT).
    • Made the calendar available via the SGX identifiers.
    • Added localized holiday labels for Singapore English (en_SG/en_US) and Thai (th).
  • Documentation

    • Updated the available financial markets list to include Singapore Exchange (SGX).
  • Tests

    • Added coverage for SGX holiday rules and localization.

Walkthrough

Adds Singapore Exchange holiday support with weekend-adjusted public holidays, Christmas Eve, New Year’s Eve, and Chinese New Year’s Eve half-days. Registers SingaporeExchange, XSES, and SGX, adds translations and documentation, and introduces comprehensive tests.

Changes

Singapore Exchange holidays

Layer / File(s) Summary
SGX holiday rules
holidays/financial/singapore_exchange.py
Defines SGX public-holiday filtering, half-day entries, market-close labels, and XSES/SGX aliases.
Exchange registration and messages
holidays/financial/__init__.py, holidays/registry.py, README.md, holidays/locale/.../XSES.po
Exports and registers the exchange codes, documents SGX, and adds English and Thai message catalogs.
Holiday behavior tests
tests/financial/test_singapore_exchange.py
Validates exchange metadata, observed half-days, localization, and 2025 full-day and half-day holiday sets.

Estimated code review effort: 3 (Moderate) | ~20 minutes

Possibly related PRs

Suggested reviewers: arkid15r, kjhellico

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Title check ✅ Passed The title clearly matches the main change: adding Singapore Exchange (SGX) holiday support.
Description check ✅ Passed The description is directly related and accurately summarizes the new SGX holidays and l10n support.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@cubic-dev-ai cubic-dev-ai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

1 issue found across 8 files

Confidence score: 4/5

  • In holidays/financial/singapore_exchange.py, weekend special closures can be added after the weekend-filtering loop, so dates like Saturday 2025-05-03 (Polling Day) may incorrectly appear in the exchange holiday set and cause downstream scheduling/trading-calendar consumers to treat weekends as special closures—reapply weekend filtering (or guard static-holiday appends) after all holiday additions.
Prompt for AI agents (unresolved issues)

Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.


<file name="holidays/financial/singapore_exchange.py">

<violation number="1" location="holidays/financial/singapore_exchange.py:37">
P2: Weekend special closures leak into the exchange calendar because Singapore's static holidays are appended after this loop. For example, `SingaporeExchange(years=2025)` includes Saturday 2025-05-03 (Polling Day), while this class otherwise removes non-trading weekend dates; filter special public holidays too.</violation>
</file>

Reply with feedback, questions, or to request a fix.

Re-trigger cubic

def _populate_public_holidays(self):
super()._populate_public_holidays()

for dt in tuple(self):

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

P2: Weekend special closures leak into the exchange calendar because Singapore's static holidays are appended after this loop. For example, SingaporeExchange(years=2025) includes Saturday 2025-05-03 (Polling Day), while this class otherwise removes non-trading weekend dates; filter special public holidays too.

Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At holidays/financial/singapore_exchange.py, line 37:

<comment>Weekend special closures leak into the exchange calendar because Singapore's static holidays are appended after this loop. For example, `SingaporeExchange(years=2025)` includes Saturday 2025-05-03 (Polling Day), while this class otherwise removes non-trading weekend dates; filter special public holidays too.</comment>

<file context>
@@ -0,0 +1,71 @@
+    def _populate_public_holidays(self):
+        super()._populate_public_holidays()
+
+        for dt in tuple(self):
+            if self._is_weekend(dt):
+                self.pop(dt)
</file context>

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

To resolve this issue, you can override _populate_common_holidays instead of _populate_public_holidays.

Comment thread holidays/locale/th/LC_MESSAGES/XSES.po Outdated

#. Chinese New Year's Eve.
msgid "Chinese New Year's Eve"
msgstr "วันสิ้นปีจีน"

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Chinese and Vietnamese New Year in Thai l10n has their own special name:

Suggested change
msgstr "วันสิ้นปีจีน"
msgstr "วันก่อนวันตรุษจีน"

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

l10n tests should be added here

@codecov

codecov Bot commented Jul 25, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 100.00%. Comparing base (3754f09) to head (329a4ec).
⚠️ Report is 3 commits behind head on dev.

Additional details and impacted files
@@            Coverage Diff            @@
##               dev     #3719   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files          321       322    +1     
  Lines        19320     19346   +26     
  Branches      2477      2479    +2     
=========================================
+ Hits         19320     19346   +26     

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

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

Copilot AI review requested due to automatic review settings July 25, 2026 18:51

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.

@sonarqubecloud

Copy link
Copy Markdown

"""Singapore Exchange (SGX) holidays.

References:
<https://web.archive.org/web/20260725102849/https://www.sgx.com/stock-exchange/trading>

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Suggested change
<https://web.archive.org/web/20260725102849/https://www.sgx.com/stock-exchange/trading>
* <https://web.archive.org/web/20260725102849/https://www.sgx.com/stock-exchange/trading>

# %s (markets close at 12:00 p.m. SGT).
pause_label = tr("%s (markets close at 12:00 p.m. SGT)")

self._move_holiday(

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Since we're removing holidays that fall on a weekend in _populate_common_holidays, there's no need to use _move_holiday here.

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.

4 participants