Add London Stock Exchange (LSE) holidays#3654
Conversation
Add the London Stock Exchange, one of the world's largest exchanges, to the supported financial markets. The LSE is closed on weekends and on the bank holidays observed in England and Wales, plus a small number of one-off royal bank holidays (jubilees, the 2011 royal wedding, the 2022 state funeral, and the 2023 coronation). Includes XLON/LSE aliases, full test coverage, registry and README entries. Output matches the library's existing England and Wales bank holiday data for 2000-2030. Add Eesh Saxena to CONTRIBUTORS.
eeshsaxena
requested review from
KJhellico,
PPsyrius and
arkid15r
as code owners
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (2)
Summary by CodeRabbit
WalkthroughAdds a London Stock Exchange financial holiday provider with XLON and LSE aliases. It updates UK holiday category gating, registers and exports the provider, adds localization and market documentation, and introduces comprehensive holiday and localization tests. ChangesLondon Stock Exchange Holiday Provider
Estimated code review effort: 3 (Moderate) | ~20 minutes Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
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. Comment |
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
1 issue found across 6 files
Confidence score: 4/5
- In
tests/financial/test_london_stock_exchange.py, the 2012 Diamond Jubilee special-case is not covered by a per-year assertion, so a regression in holiday logic could slip through CI and produce incorrect market-closure dates for that year—add a dedicated 2012 test case before merging to de-risk it.
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="tests/financial/test_london_stock_exchange.py">
<violation number="1" location="tests/financial/test_london_stock_exchange.py:134">
P2: Missing per-year test for 2012 Diamond Jubilee, leaving special-cased holidays untested.</violation>
</file>
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
|
If holidays are the same as in UK, why not just inherit from |
|
Good question, and I did try that first. The blocker is that the LSE follows specifically the England and Wales bank holidays, which in I prototyped To make sure the standalone version is faithful, I cross-checked its output against Happy to go whichever way you prefer:
|
Is this what you are looking for? |
|
Thanks @arkid15r, that's exactly the pointer I needed. I tried it: class LondonStockExchange(ChildEntity, UnitedKingdom):
country = None
market = "XLON"
parent_entity = UnitedKingdom
parent_entity_subdivision_code = "ENG"
supported_categories = (PUBLIC,)
start_year = 2000The output matches The one thing I'm stuck on is Is there an additional step to make a subdivision-backed market not expose |
| market = "XLON" | ||
| observed_label = "%s (observed)" | ||
| start_year = 2000 |
There was a problem hiding this comment.
@eeshsaxena the LSE also observes early closures/half days (like Christmas Eve and New Year's Eve). Here is the reference link
We will need to add half-day support to make this fully accurate. I've already done the research on this, so I am happy to help you implement this section if you'd like! Just let me know
The London Stock Exchange runs a shortened session with an early close on Christmas Eve and New Year's Eve. These are now exposed under the HALF_DAY category, following the existing exchange half-day pattern.
|
Thanks @pareshjoshij! Added half-day support - Christmas Eve and New Year's Eve early closes are now exposed under the |
There was a problem hiding this comment.
All reported issues were addressed across 2 files (changes from recent commits).
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
|
@eeshsaxena how will the end user know that these dates are half-days?
take a look at the recent bot comment on this PR. For the best approach, I suggest reviewing #3617 to see exactly how we implemented half-day holidays previously, and let's align with that pattern. :) |
pareshjoshij
left a comment
pareshjoshij
left a comment
There was a problem hiding this comment.
In my opinion, it would be highly beneficial to cross-reference your output with historical LSE calendars. For example, you can use the Wayback Machine to look at past versions of their business days page: LSE Business Days (Archive).
Here is why I suggest doing this:
- Accuracy Check: Matching the official historical holidays against your generated output will give you a clear idea of whether your current implementation is accurate, or if inheriting from
UnitedKingdommight actually be a better approach. - Easier Reviews: If you add these historical links to the
Referencessection in the docstring, it significantly reduces the workload for maintainers when verifying the dates.
I highly recommend digging up a few of these historical links yourself—it is a great way to learn the intricacies of the exchange's calendar! If you need a good research strategy for finding them, check out this trick shared by @PPsyrius in a previous review: PR #3639 Research Tip.
Just my two cents—hope this helps!
Look at similar markets that derive from countries - JPX, SSE etc. |
KJhellico
changed the title
|
Thanks both! @pareshjoshij - done. Following the #3617 (XCME) pattern, the half-day sessions are now labelled via so end users can clearly tell these are shortened trading days (12:30 close) rather than full market holidays. @KJhellico - on inheriting from So the LSE specifically needs |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
tests/financial/test_london_stock_exchange.py (1)
89-104: 📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick winMissing 2012 Jubilee-year test.
The PR notes Spring Bank Holiday is moved for 2002, 2012, and 2022 (Jubilee years), but tests only cover 2002 and 2022. Add a
test_2012to verify the Diamond Jubilee adjustment (Spring Bank Holiday moved to June 4) matches the implementation'sspring_bank_datesmapping.✅ Suggested test addition
def test_2012(self): self.assertHolidays( LondonStockExchange(years=2012), ("2012-01-01", "New Year's Day"), # adjust as needed for actual 2012 dates/observed rules ("2012-04-06", "Good Friday"), ("2012-04-09", "Easter Monday"), ("2012-05-07", "May Day"), ("2012-06-04", "Spring Bank Holiday"), ("2012-06-05", "Diamond Jubilee of Elizabeth II"), # verify actual static holiday name ("2012-08-27", "Late Summer Bank Holiday"), ("2012-12-25", "Christmas Day"), ("2012-12-26", "Boxing Day"), )🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/financial/test_london_stock_exchange.py` around lines 89 - 104, Add a missing 2012 coverage case in the LondonStockExchange holiday tests so the Jubilee-year Spring Bank Holiday shift is verified alongside 2002 and 2022. Update the test class in test_london_stock_exchange to include a new test_2012 that asserts the 2012 holiday set, especially that Spring Bank Holiday falls on June 4 per the spring_bank_dates mapping and includes the Diamond Jubilee observance used by the implementation.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@tests/financial/test_london_stock_exchange.py`:
- Around line 89-104: Add a missing 2012 coverage case in the
LondonStockExchange holiday tests so the Jubilee-year Spring Bank Holiday shift
is verified alongside 2002 and 2022. Update the test class in
test_london_stock_exchange to include a new test_2012 that asserts the 2012
holiday set, especially that Spring Bank Holiday falls on June 4 per the
spring_bank_dates mapping and includes the Diamond Jubilee observance used by
the implementation.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 403624ae-336e-4718-8e25-eaf782eb56be
📒 Files selected for processing (2)
holidays/financial/london_stock_exchange.pytests/financial/test_london_stock_exchange.py
I was talking exactly about this test, not the implementation. Of course, for the implementation itself, this parent_entity_subdivision_code = "ENG"is needed. Also, for the HALF_DAY category to work properly, you need to add diff --git a/holidays/countries/united_kingdom.py b/holidays/countries/united_kingdom.py
--- a/holidays/countries/united_kingdom.py (revision b0d20a82c4b8fd2001cb3b3fb9fabfd8f491b263)
+++ b/holidays/countries/united_kingdom.py (date 1783183559104)
@@ -13,6 +13,7 @@
from gettext import gettext as tr
from holidays.calendars.gregorian import APR, MAY, JUN, JUL, SEP, DEC
+from holidays.constants import PUBLIC
from holidays.groups import ChristianHolidays, InternationalHolidays, StaticHolidays
from holidays.observed_holiday_base import (
ObservedHolidayBase,
@@ -104,7 +105,7 @@
self._add_whit_monday(tr("Whit Monday"))
def _populate_subdiv_holidays(self):
- if self.subdiv != "SCT":
+ if PUBLIC in self.categories and self.subdiv != "SCT":
if self._year >= 1975:
# New Year's Day.
self._add_observed(self._add_new_years_day(tr("New Year's Day")))
BTW, since the United Kingdom supports l10n, it would be advisable to implement it here as well. |
Reworks the London Stock Exchange to derive from UnitedKingdom via ChildEntity with parent_entity_subdivision_code="ENG" (following the JPX/SSE pattern), instead of duplicating the England bank-holiday logic. Its public holidays are now an exact alias of UnitedKingdom(subdiv="ENG"), including the one-off royal bank holidays, with no duplicated code. To keep the HALF_DAY category clean, gate UnitedKingdom's shared subdivision public holidays on `PUBLIC in self.categories`, so requesting only HALF_DAY no longer pulls in New Year's Day / Christmas / Boxing Day. Adds localization (en_GB source, en_US, th) for the half-day session strings and lists XLON's languages and HALF_DAY category in the README.
|
Thanks @KJhellico - refactored to the inheritance approach as you suggested. Summary of the changes: 1. Inherit from 2. 3. HALF_DAY guard. Applied your - if self.subdiv != "SCT":
+ if PUBLIC in self.categories and self.subdiv != "SCT":so requesting only 4. l10n. Added localization for the half-day strings (source README updated with XLON's languages +
|
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
tests/financial/test_london_stock_exchange.py (1)
36-151: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winYear-specific holidays checked against UK observance rules — all dates and observed shifts look correct for 2002, 2011, 2020-2025.
However, the PR objectives call out Jubilee-year Spring Bank Holiday adjustments in 2002, 2012, and 2022, but only 2002 and 2022 are tested here — the 2012 Diamond Jubilee shift has no coverage.
♻️ Suggested addition
def test_2012(self): self.assertHolidays( LondonStockExchange(years=2012), ("2012-01-01", "New Year's Day"), ("2012-01-02", "New Year's Day (observed)"), ("2012-04-06", "Good Friday"), ("2012-04-09", "Easter Monday"), ("2012-05-07", "May Day"), ("2012-06-04", "Spring Bank Holiday"), ("2012-06-05", "Diamond Jubilee of Elizabeth II"), ("2012-08-27", "Late Summer Bank Holiday"), ("2012-12-25", "Christmas Day"), ("2012-12-26", "Boxing Day"), )🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/financial/test_london_stock_exchange.py` around lines 36 - 151, Add missing coverage in LondonStockExchange tests for the 2012 Jubilee year by introducing a test_2012 alongside test_2002/test_2022. Use LondonStockExchange(years=2012) in test_london_stock_exchange.py and assert the Diamond Jubilee-related holiday set, including the shifted Spring Bank Holiday and observed New Year’s Day, so the year-specific observance rules are exercised for 2012 as well.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@tests/financial/test_london_stock_exchange.py`:
- Around line 36-151: Add missing coverage in LondonStockExchange tests for the
2012 Jubilee year by introducing a test_2012 alongside test_2002/test_2022. Use
LondonStockExchange(years=2012) in test_london_stock_exchange.py and assert the
Diamond Jubilee-related holiday set, including the shifted Spring Bank Holiday
and observed New Year’s Day, so the year-specific observance rules are exercised
for 2012 as well.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: f3c1c613-f143-4394-8602-206398da547b
📒 Files selected for processing (7)
README.mdholidays/countries/united_kingdom.pyholidays/financial/london_stock_exchange.pyholidays/locale/en_GB/LC_MESSAGES/XLON.poholidays/locale/en_US/LC_MESSAGES/XLON.poholidays/locale/th/LC_MESSAGES/XLON.potests/financial/test_london_stock_exchange.py
| def test_market_aliases(self): | ||
| self.assertAliases(LondonStockExchange, XLON, LSE) | ||
|
|
||
| def test_no_holidays(self): | ||
| self.assertNoHolidays(LondonStockExchange(years=1999)) | ||
|
|
There was a problem hiding this comment.
| def test_market_aliases(self): | |
| self.assertAliases(LondonStockExchange, XLON, LSE) | |
| def test_no_holidays(self): | |
| self.assertNoHolidays(LondonStockExchange(years=1999)) |
Simplify the class docstring, rename the half-day label to 'markets close at 12:30pm' matching the requested phrasing, and consolidate the yearly test cases into a single representative year plus dedicated Christmas Eve/New Year's Eve and l10n tests.
|
Thanks @KJhellico - applied all of it:
Also added `-> None` to `_populate_half_day_holidays` per CodeRabbit's nitpick. Left the CodeRabbit "missing 2012 Jubilee test" suggestion out since it conflicts with consolidating to one yearly test - happy to add it back if you'd rather keep both. Ruff, `test_l10n`, `test_docs`, the LSE/UK suites (14 + 43 tests) all green locally. |
|
Caution Failed to replace (edit) comment. This is likely due to insufficient permissions or the comment being deleted. Error details |
- "Project-Id-Version: Holidays 0.100\n"
+ "Project-Id-Version: Holidays 0.101\n"
|
| # Exercises the royal one-off bank holidays alongside the regular | ||
| # England & Wales calendar; year-by-year correctness of the underlying | ||
| # calendar itself is covered by the United Kingdom test suite. | ||
| self.assertHolidays( | ||
| LondonStockExchange(years=2022), |
There was a problem hiding this comment.
| # Exercises the royal one-off bank holidays alongside the regular | |
| # England & Wales calendar; year-by-year correctness of the underlying | |
| # calendar itself is covered by the United Kingdom test suite. | |
| self.assertHolidays( | |
| LondonStockExchange(years=2022), | |
| self.assertHolidaysInYear( | |
| 2022, |
An place this test before l10n tests.
There was a problem hiding this comment.
@eeshsaxena, the suggestion includes removing the redundant comment.
And "before" means "right before". ;)
|
Thanks both - done:
14 LSE tests plus |
pareshjoshij
left a comment
pareshjoshij
left a comment
There was a problem hiding this comment.
- Unshifted Half-Days: When Christmas Eve (Dec 24) and New Year's Eve (Dec 31) fall on a weekend, the LSE shifts the half-day early close to the preceding Friday. The current script hardcodes these to the 24th and 31st, resulting in market opens being reported on Saturdays or Sundays.
- Because the class inherits from the
UnitedKingdomcalendar, it emits standard weekend holidays (e.g., actual Christmas Day on a Sunday) alongside the observed weekday substitutes. IMO it should only output affected business days .
Comparison (Using 2011 as an example)
In 2011, Dec 24 and Dec 31 fell on Saturdays. The official LSE schedule shifted the early closes to Friday, Dec 23 and Friday, Dec 30.
| Current Output | Official LSE Schedule |
|---|---|
2011-01-01 -> New Year's Day |
(Removed - Weekend) |
2011-01-03 -> New Year's Day (observed) |
2011-01-03 -> New Year's Day (observed) |
2011-04-22 -> Good Friday |
2011-04-22 -> Good Friday |
2011-04-25 -> Easter Monday |
2011-04-25 -> Easter Monday |
2011-04-29 -> Wedding of William and Catherine |
2011-04-29 -> Wedding of William and Catherine |
2011-05-02 -> May Day |
2011-05-02 -> May Day |
2011-05-30 -> Spring Bank Holiday |
2011-05-30 -> Spring Bank Holiday |
2011-08-29 -> Late Summer Bank Holiday |
2011-08-29 -> Late Summer Bank Holiday |
2011-12-24 -> Christmas Eve (markets close at 12:30pm) |
2011-12-23 -> Christmas Eve (markets close at 12:30pm) |
2011-12-25 -> Christmas Day |
(Removed - Weekend) |
2011-12-26 -> Boxing Day |
2011-12-26 -> Boxing Day |
2011-12-27 -> Christmas Day (observed) |
2011-12-27 -> Christmas Day (observed) |
2011-12-31 -> New Year's Eve (markets close at 12:30pm) |
2011-12-30 -> New Year's Eve (markets close at 12:30pm) |
|
Thanks @pareshjoshij, this is a great catch and your 2011 reference is spot on. I reproduced both issues on the current branch: There are really two separate things here, and I want to get the second one right rather than guess: 1. Half-day early closes falling on a weekend. This one is clearly a bug - an early close on a Saturday is meaningless. I can fix it by shifting the half-day to the preceding Friday, which matches your 2011 evidence (Dec 24 Sat -> Dec 23 Fri, Dec 31 Sat -> Dec 30 Fri). The library already has 2. Weekend-falling public holidays. I agree a market calendar arguably shouldn't list Christmas Day when it lands on a Sunday (the market is already closed, and the observed substitute is what actually matters). The wrinkle is that these come from the inherited So before I implement #2: @KJhellico @pareshjoshij, what is the direction you'd prefer - filter out inherited holidays that fall on a weekend (drop the raw Sat/Sun dates, keep the observed substitutes), or keep the full inherited set as-is? I'll implement whichever you both agree on. I can push the half-day fix (#1) right away since that one seems uncontroversial. |
Don't worry buddy, we're in the same boat! Take a look at my open PR #3692 below and just read through the mentor's suggestions |
The exchange does not trade at weekends, so a bank holiday falling on a Saturday or Sunday is not a trading day in its own right; only its observed weekday substitute is. Suppress those inherited dates with SAT_TO_NONE + SUN_TO_NONE, following the approach used for SIX. The shortened Christmas Eve and New Year's Eve sessions likewise move back to the preceding Friday when the eve falls on a weekend, matching the published LSE calendar (2011: Dec 23 and Dec 30).
|
Thanks @pareshjoshij - that pointer was exactly what I needed, and thanks @KJhellico for the guidance on #3692. Both of your points are now implemented. 1. Weekend bank holidays are dropped. Following the approach @KJhellico prescribed on #3692, the inherited England and Wales dates that land on a Saturday or Sunday are suppressed, so only the observed weekday substitutes remain: def _populate(self, year: int) -> None:
super()._populate(year)
if self.observed:
for dt in tuple(self.keys()):
if dt.year == year:
self._move_holiday(dt, rule=SAT_TO_NONE + SUN_TO_NONE, show_observed_label=False)I used the composed 2. Half-days move to the preceding Friday. The shortened Christmas Eve and New Year's Eve sessions now shift back via 2011 now matches the official schedule you linked, exactly:
Tests: updated Agreed with @KJhellico that a cleaner mechanism for country-backed markets would be nice down the line - happy to help with that once the pattern settles. |
|
| for dt in ( | ||
| self._add_christmas_eve( | ||
| # Christmas Eve. | ||
| self._format_holiday_name(close_12_30pm_label, tr("Christmas Eve")) | ||
| ), | ||
| self._add_new_years_eve( | ||
| # New Year's Eve. | ||
| self._format_holiday_name(close_12_30pm_label, tr("New Year's Eve")) | ||
| ), | ||
| ): | ||
| # The shortened session moves to the preceding Friday when the eve falls | ||
| # on a weekend, since the exchange does not trade on Saturday or Sunday. | ||
| self._move_holiday(dt, rule=SAT_SUN_TO_PREV_FRI, show_observed_label=False) |
There was a problem hiding this comment.
| for dt in ( | |
| self._add_christmas_eve( | |
| # Christmas Eve. | |
| self._format_holiday_name(close_12_30pm_label, tr("Christmas Eve")) | |
| ), | |
| self._add_new_years_eve( | |
| # New Year's Eve. | |
| self._format_holiday_name(close_12_30pm_label, tr("New Year's Eve")) | |
| ), | |
| ): | |
| # The shortened session moves to the preceding Friday when the eve falls | |
| # on a weekend, since the exchange does not trade on Saturday or Sunday. | |
| self._move_holiday(dt, rule=SAT_SUN_TO_PREV_FRI, show_observed_label=False) | |
| self._move_holiday( | |
| self._add_christmas_eve( | |
| # Christmas Eve. | |
| self._format_holiday_name(close_12_30pm_label, tr("Christmas Eve")) | |
| ), | |
| rule=SAT_SUN_TO_PREV_FRI, | |
| show_observed_label=False, | |
| ) | |
| self._move_holiday( | |
| self._add_new_years_eve( | |
| # New Year's Eve. | |
| self._format_holiday_name(close_12_30pm_label, tr("New Year's Eve")) | |
| ), | |
| rule=SAT_SUN_TO_PREV_FRI, | |
| show_observed_label=False, | |
| ) |
It's advisable to simply expand on the existing comment about Christmas Eve and New Year's Eve in docstring.
| # The exchange is closed at weekends, so a bank holiday that falls on a | ||
| # Saturday or Sunday is not a trading day in its own right; only its | ||
| # observed weekday substitute is. |
There was a problem hiding this comment.
I'm not sure whether such a detailed comment is necessary, but I hope that in the end we'll get completely rid of this code fragment.
4 participants
Proposed change
Adds the London Stock Exchange (
XLON) to the supported financial markets. The LSE is one of the world's largest exchanges and was not yet covered, while several smaller exchanges already are.The LSE is closed on weekends and on the bank holidays observed in England and Wales:
plus the one-off royal bank holidays: the 2002 Golden Jubilee, the 2011 royal wedding, the 2012 Diamond Jubilee, the 2022 Platinum Jubilee, the 2022 State Funeral of Queen Elizabeth II, and the 2023 Coronation of Charles III.
start_yearis 2000 (the modern electronic-trading era).Verification
tests/financial/test_london_stock_exchange.py(per-year assertions, aliases, and a no-holidays check) - all pass, 100% coverage of the new module.UnitedKingdom(subdiv="ENG")bank-holiday data for every year 2000-2030 (exact match on both dates and names).tests/test_docs.pyandtests/test_registry.pypass; README financial-markets table and registry updated;ruff check/formatclean.Aliases
LondonStockExchange,XLON,LSE.