More Markets #3
More Markets #3
Conversation
WalkthroughThis update introduces significant enhancements to the BetterHolidays package. It adds support for new markets (NASDAQ, SSE, LSE), implements database-backed caching via Peewee ORM, and enriches holiday handling with dynamic date fetching and new utility functions. The Market class is refactored for extensibility, and new modules and dependencies are included to support these features. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant Market
participant Cache
participant Database
participant HolidayScraper
User->>Market: day(date)
Market->>Cache: get(date)
Cache->>Database: Query Day by date & market
alt Found in DB
Database-->>Cache: Return Day
Cache-->>Market: Return Day
else Not found
Cache-->>Market: Raise exception
Market->>Market: fetch_data(year)
Market->>HolidayScraper: (if needed) Scrape holiday data
HolidayScraper-->>Market: Return holiday dates
Market->>Cache: set(Day)
Cache->>Database: Save Day
Market->>Cache: get(date)
Cache->>Database: Query Day by date & market
Database-->>Cache: Return Day
Cache-->>Market: Return Day
end
Market-->>User: Return Day
Possibly related PRs
Poem
Tip ⚡️ Faster reviews with caching
Enjoy the performance boost—your workflow just got faster. ✨ Finishing Touches
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 19
🔭 Outside diff range comments (2)
BetterHolidays/markets/lse.py (1)
27-57: 🛠️ Refactor suggestionImprove exception handling in fetch_data method.
The exception handling should use
raise ... from eto properly chain exceptions. This helps distinguish the original error from the new one.except Exception as e: - raise ValueError(f"Error fetching LSE data for {year}: {e}") + raise ValueError(f"Error fetching LSE data for {year}: {e}") from eAlso, consider adding a docstring to the method explaining its purpose and return value.
🧰 Tools
🪛 Ruff (0.8.2)
54-54: Within an
exceptclause, raise exceptions withraise ... from errorraise ... from Noneto distinguish them from errors in exception handling(B904)
BetterHolidays/markets/cache.py (1)
67-69:⚠️ Potential issue
__contains__usesinon a model instance – logic error
key in Day.select().where(...)will raiseTypeError. Usequery.exists().- return key in Day.select().where(Day.market == self.market, Day.date == key).get() + return ( + Day.select() + .where(Day.market == self.market, Day.date == key) + .exists() + )
🧹 Nitpick comments (11)
BetterHolidays/markets/__init__.py (2)
8-8: ExpandMARKETSregistry.
Including the new markets in theMARKETSdict ensures dynamic access by name. Consider alphabetizing keys for easier maintenance.Example:
- MARKETS: dict[str, type[Market]] = {"NYSE": NYSE, "NASDAQ": NASDAQ, "SSE": SSE, "LSE": LSE} + MARKETS: dict[str, type[Market]] = { + "LSE": LSE, + "NASDAQ": NASDAQ, + "NYSE": NYSE, + "SSE": SSE, + }
10-10: Review__all__ordering and consistency.
The__all__list exports all market classes including the new ones. For readability, sorting entries alphabetically can help future contributors.test.py (1)
1-20: Good demonstration of the classproperty functionality.This test file effectively demonstrates the getter, setter, and deleter functionality of the
classpropertydecorator. It showcases how to define a class property, modify its value, and handle deletion.However, I would recommend adding a docstring to explain the purpose of this test file and consider moving it to a proper test directory if this is intended to be part of the test suite.
BetterHolidays/markets/lse.py (1)
10-22: Market class definition looks good, but could use a docstring.The LSE class definition with its attributes and regex pattern is well-structured, but adding a docstring would help explain the purpose and behavior of this class.
class LSE(Market): + """ + London Stock Exchange (LSE) market implementation. + + This class handles trading days and holidays for the London Stock Exchange. + """ name = "London Stock Exchange" country = "UK" include_country_holidays = True excluded_country_holidays = []BetterHolidays/markets/nasdaq.py (2)
106-108: Drop the redundantf-prefixThe formatted-string literal has no placeholders; Ruff flags this (F541).
- page = md.HTML.from_url( - f"https://www.nasdaq.com/market-activity/stock-market-holiday-schedule" - ) + page = md.HTML.from_url( + "https://www.nasdaq.com/market-activity/stock-market-holiday-schedule" + )🧰 Tools
🪛 Ruff (0.8.2)
107-107: f-string without any placeholders
Remove extraneous
fprefix(F541)
35-35:abnormal_daysis declared but never populated or consumedIf this dict is meant for run-time overrides it needs to be integrated (e.g. checked in
day()lookup) otherwise it is dead code. Consider deleting it until it is actually required.BetterHolidays/utils.py (1)
86-96: Avoid single-letter variable names and E741 (“ambiguous ‘l’”)Rename
ltoitems(or similar) to improve readability and silence the Ruff warning.-def flatten_list(l:'list[list[T]|T1]', recursive:bool=False) -> 'list[T1|T]': - ret = [] - for item in l: +def flatten_list(items: 'list[list[T] | T1]', recursive: bool = False) -> 'list[T1 | T]': + ret: list[T | T1] = [] + for item in items:🧰 Tools
🪛 Ruff (0.8.2)
86-86: Ambiguous variable name:
l(E741)
BetterHolidays/markets/market.py (1)
124-150: Minor simplification: return boolean expressions directlyYou can return the
isinstanceresult instead of using anif/returnpair.
This removes three lines of boilerplate per method.- day = cls.day(date) - if not isinstance(day, Holiday): - return False - return True + return isinstance(cls.day(date), Holiday)Same applies to
is_partial_dayandis_trading_day.🧰 Tools
🪛 Ruff (0.8.2)
134-136: Return the condition
isinstance(day, Holiday)directlyReplace with
return isinstance(day, Holiday)(SIM103)
141-143: Return the condition
isinstance(day, PartialTradingDay)directlyReplace with
return isinstance(day, PartialTradingDay)(SIM103)
148-150: Return the condition
isinstance(day, TradingDay)directlyReplace with
return isinstance(day, TradingDay)(SIM103)
BetterHolidays/markets/holidays.py (3)
3-3: Remove unused importdefault.The symbol
defaultis imported but never referenced in this module, which triggers Ruff F401 and clutters the namespace.-from ..utils import flatten_list, default +from ..utils import flatten_list🧰 Tools
🪛 Ruff (0.8.2)
3-3:
..utils.defaultimported but unusedRemove unused import:
..utils.default(F401)
94-98: Combine nestedifstatements for clarity (Ruff SIM102).- if day.weekday() not in self.days: - if day.weekday() in self.change: - day += dt.timedelta(days=self.change[day.weekday()]) + if day.weekday() not in self.days and day.weekday() in self.change: + day += dt.timedelta(days=self.change[day.weekday()])🧰 Tools
🪛 Ruff (0.8.2)
95-96: Use a single
ifstatement instead of nestedifstatements(SIM102)
298-301: Preserve exception context when re-raising (B904).Re-raising with
from errkeeps the original traceback and avoids masking root causes:- return dt.date(year, MONTHS_MAP[month.upper()], int(day)) - except Exception as e: - raise ValueError( - f"Error determining Good Friday date for {year}: {str(e)} ({type(e)})" - ) + return dt.date(year, MONTHS_MAP[month.upper()], int(day)) + except Exception as err: + raise ValueError( + f"Error determining Good Friday date for {year}: {err} ({type(err).__name__})" + ) from err🧰 Tools
🪛 Ruff (0.8.2)
299-301: Within an
exceptclause, raise exceptions withraise ... from errorraise ... from Noneto distinguish them from errors in exception handling(B904)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (16)
BetterHolidays/__init__.py(2 hunks)BetterHolidays/const.py(3 hunks)BetterHolidays/days.py(1 hunks)BetterHolidays/markets/__init__.py(1 hunks)BetterHolidays/markets/cache.py(1 hunks)BetterHolidays/markets/holidays.py(3 hunks)BetterHolidays/markets/lse.py(1 hunks)BetterHolidays/markets/market.py(2 hunks)BetterHolidays/markets/nasdaq.py(1 hunks)BetterHolidays/markets/nyse.py(1 hunks)BetterHolidays/markets/sse.py(1 hunks)BetterHolidays/multi.py(1 hunks)BetterHolidays/utils.py(2 hunks)requirements.txt(1 hunks)setup.py(1 hunks)test.py(1 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (4)
test.py (1)
BetterHolidays/utils.py (2)
classproperty(44-65)delete(54-56)
BetterHolidays/days.py (1)
BetterHolidays/utils.py (1)
get_db(40-41)
BetterHolidays/utils.py (1)
BetterHolidays/markets/market.py (1)
days(164-165)
BetterHolidays/markets/market.py (7)
BetterHolidays/days.py (5)
Day(5-12)Holiday(14-17)TradingDay(20-24)PartialTradingDay(33-39)NonTradingDay(27-30)BetterHolidays/utils.py (3)
iterate_date(75-79)iter_year(81-84)default(98-99)BetterHolidays/markets/cache.py (2)
Cache(12-68)get_or_set(23-32)BetterHolidays/markets/lse.py (2)
fetch_data(28-56)weekdays(24-25)BetterHolidays/markets/nasdaq.py (2)
fetch_data(94-98)weekdays(90-91)BetterHolidays/markets/nyse.py (2)
fetch_data(184-188)weekdays(180-181)BetterHolidays/markets/sse.py (1)
weekdays(34-35)
🪛 Ruff (0.8.2)
BetterHolidays/markets/lse.py
23-23: Undefined name classproperty
(F821)
54-54: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
BetterHolidays/markets/cache.py
32-32: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
52-52: Do not use bare except
(E722)
53-53: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
59-59: Do not use bare except
(E722)
64-64: Local variable e is assigned to but never used
Remove assignment to unused variable e
(F841)
65-65: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
BetterHolidays/markets/nasdaq.py
10-10: .holidays.LincolnsBirthday imported but unused
Remove unused import: .holidays.LincolnsBirthday
(F401)
107-107: f-string without any placeholders
Remove extraneous f prefix
(F541)
121-121: Undefined name year
(F821)
148-148: Undefined name year
(F821)
BetterHolidays/markets/sse.py
3-3: ..days.Holiday imported but unused
Remove unused import: ..days.Holiday
(F401)
4-4: ..const.MONTHS_MAP imported but unused
Remove unused import: ..const.MONTHS_MAP
(F401)
5-5: BetterMD imported but unused
Remove unused import: BetterMD
(F401)
33-33: Undefined name classproperty
(F821)
BetterHolidays/utils.py
86-86: Ambiguous variable name: l
(E741)
BetterHolidays/markets/market.py
134-136: Return the condition isinstance(day, Holiday) directly
Replace with return isinstance(day, Holiday)
(SIM103)
141-143: Return the condition isinstance(day, PartialTradingDay) directly
Replace with return isinstance(day, PartialTradingDay)
(SIM103)
148-150: Return the condition isinstance(day, TradingDay) directly
Replace with return isinstance(day, TradingDay)
(SIM103)
BetterHolidays/markets/holidays.py
3-3: ..utils.default imported but unused
Remove unused import: ..utils.default
(F401)
95-96: Use a single if statement instead of nested if statements
(SIM102)
186-186: Undefined name date
(F821)
293-293: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
299-301: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
BetterHolidays/markets/nyse.py
219-219: Function definition does not bind loop variable year
(B023)
🔇 Additional comments (17)
requirements.txt (1)
1-2:Details
✅ Verification successful
Verify dependency alignment across files.
The newplatformdirs>=4.0.0entry must match the dependency insetup.pyand support your database caching implementation inBetterHolidays/utils.py. Please ensure both files stay in sync to avoid mismatched environments.Run this script to cross-check versions:
🏁 Script executed:
#!/bin/bash # Verify presence and versions of key dependencies in both files grep -E 'platformdirs|better-md' requirements.txt setup.pyLength of output: 208
Dependency versions are in sync
Verified that both requirements.txt and setup.py specifybetter-md>=0.3.4andplatformdirs>=4.0.0. No mismatches found—no further action needed.setup.py (1)
18-21:Details
✅ Verification successful
Ensure
install_requiresmatchesrequirements.txt.
You've added"platformdirs>=4.0.0"underinstall_requires. Confirm that this lines up exactly with yourrequirements.txtentry and that no other dependencies are omitted.Use the following command to verify:
🏁 Script executed:
#!/bin/bash grep -E 'platformdirs|better-md' requirements.txt setup.pyLength of output: 208
install_requires matches requirements.txt
All dependencies (better-md>=0.3.4,platformdirs>=4.0.0) are identical in both setup.py and requirements.txt.BetterHolidays/const.py (5)
3-3: Formatting update forDAYStuple.
Consolidating the weekdays into a single-line tuple improves readability without affecting functionality.
6-8: Standardize day constant declarations.
The assignments forMONDAY–SUNDAYand theDAYS_TYPEannotation have been reformatted for consistency. Behavior remains unchanged.
21-21: Add trailing comma for multi-line tuple.
Including a trailing comma after"DECEMBER"inMONTHSis a good practice for cleaner diffs and future edits.
25-38: Reformat month constants assignment.
Switching to a multi-line unpacking ofrange(1, 13)maintains the same values while enhancing visual structure.
53-53: Add trailing comma to dictionary entry.
Adding a trailing comma after the lastDAYS_IN_MONTHentry keeps the style consistent and eases future modifications.BetterHolidays/__init__.py (2)
3-3: Expose new markets in package-level imports.
ImportingNASDAQ,SSE, andLSEalongside existing markets ensures they are available through the top-level API. No circular import issues detected.
13-18: Update__all__to include additional markets.
The public API now correctly exposesNASDAQ,SSE,LSE, andget_market. The trailing comma and newline addition align with PEP-8 recommendations.BetterHolidays/markets/__init__.py (1)
4-6:Details
✅ Verification successful
Import new market classes.
AddingNASDAQ,SSE, andLSEhere aligns with the new submodules. Ensure thatnasdaq.py,sse.py, andlse.pydefine these classes exactly.Run:
🏁 Script executed:
#!/bin/bash grep -R "class NASDAQ" -n BetterHolidays/markets grep -R "class SSE" -n BetterHolidays/markets grep -R "class LSE" -n BetterHolidays/marketsLength of output: 308
Imports Verified: NASDAQ, SSE, and LSE classes are present in their modules
grep confirms:
- BetterHolidays/markets/nasdaq.py:24 ⇒
class NASDAQ(Market):- BetterHolidays/markets/sse.py:10 ⇒
class SSE(Market):- BetterHolidays/markets/lse.py:10 ⇒
class LSE(Market):No further changes needed here.
BetterHolidays/multi.py (2)
9-16: Consistent type annotation style improvement.The changes to the type annotations, using double quotes instead of single quotes for forward references, improve consistency with the rest of the codebase. These types of style standardizations make the code more maintainable.
21-25: Whitespace improvements for better readability.Adding blank lines between logical sections of the code improves readability and follows common Python style guidelines.
BetterHolidays/markets/lse.py (1)
23-26: Classproperty usage is correct, pending import fix.The implementation of the weekdays property is correct, returning the days Monday to Friday (0-4).
🧰 Tools
🪛 Ruff (0.8.2)
23-23: Undefined name
classproperty(F821)
BetterHolidays/markets/sse.py (4)
16-20: Timezone definition looks good.The timezone definition and trading hours are well-defined using the zoneinfo module.
23-31: Holiday definitions are comprehensive.The holiday definitions for the Shanghai Stock Exchange are comprehensive, covering major Chinese holidays with appropriate weekday adjustments.
33-35: Classproperty implementation is correct, pending import fix.The weekdays property correctly indicates Monday to Friday as trading days.
🧰 Tools
🪛 Ruff (0.8.2)
33-33: Undefined name
classproperty(F821)
1-35:Details
❌ Incorrect review comment
File appears incomplete.
The file seems to be missing implementation for fetching or processing holiday data, unlike the LSE implementation. Check if this is intentional or if the implementation relies on the holiday classes to handle this.
🏁 Script executed:
#!/bin/bash # Check if there's a missing fetch_data method or if the holiday classes handle the data fetching echo "Checking for fetch_data implementation in parent class:" rg -A 5 "def fetch_data" BetterHolidays/markets/market.py echo -e "\nChecking how holiday classes fetch data:" rg -A 5 "def get_date|def get_dates" BetterHolidays/markets/holidays.pyLength of output: 4597
No missing implementation in SSE: holiday classes provide the data fetching
The SSE class inheritsfetch_datafromMarket, which in turn calls each holiday’sget_dateimplementation. All holiday classes (e.g.ChineseNewYearsDay,QingMingFestival, etc.) defineget_dateby fetching or computing their dates, so no additional methods are required inSSE.
No changes needed.Likely an incorrect or invalid review comment.
🧰 Tools
🪛 Ruff (0.8.2)
3-3:
..days.Holidayimported but unusedRemove unused import:
..days.Holiday(F401)
4-4:
..const.MONTHS_MAPimported but unusedRemove unused import:
..const.MONTHS_MAP(F401)
5-5:
BetterMDimported but unusedRemove unused import:
BetterMD(F401)
33-33: Undefined name
classproperty(F821)
| def last_day(day: "DAYS_TYPE", want: "DAYS_TYPE") -> "DAYS_TYPE": | ||
| """ | ||
| Args: | ||
| day: current day | ||
| want: day to get | ||
| Returns: | ||
| days until want | ||
| """ | ||
| return next_day(day, want) | ||
|
|
There was a problem hiding this comment.
last_day() returns the wrong offset (always “forward” instead of “back”)
last_day() should give the distance to the previous occurrence of want, but the current implementation simply delegates to next_day(), producing an offset in the wrong direction (e.g. from Wednesday to Sunday it returns 4 instead of 3).
This mis-calculates many holiday ranges (e.g. Chinese New Year’s Eve) and makes last_weekend() behave erratically.
-def last_day(day: "DAYS_TYPE", want: "DAYS_TYPE") -> "DAYS_TYPE":
- """
- Args:
- day: current day
- want: day to get
-
- Returns:
- days until want
- """
- return next_day(day, want)
+def last_day(day: "DAYS_TYPE", want: "DAYS_TYPE") -> "DAYS_TYPE":
+ """
+ Days to step *backwards* from ``day`` to the previous ``want``.
+ Always non-negative so it can safely be used in ``datetime.timedelta``.
+ """
+ if day == want:
+ return 0
+ if want > day:
+ # Wrap backwards over the week boundary
+ return (day + 7) - want
+ # Same week, simply walk back
+ return day - want📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| def last_day(day: "DAYS_TYPE", want: "DAYS_TYPE") -> "DAYS_TYPE": | |
| """ | |
| Args: | |
| day: current day | |
| want: day to get | |
| Returns: | |
| days until want | |
| """ | |
| return next_day(day, want) | |
| def last_day(day: "DAYS_TYPE", want: "DAYS_TYPE") -> "DAYS_TYPE": | |
| """ | |
| Days to step *backwards* from ``day`` to the previous ``want``. | |
| Always non-negative so it can safely be used in ``datetime.timedelta``. | |
| """ | |
| if day == want: | |
| return 0 | |
| if want > day: | |
| # Wrap backwards over the week boundary | |
| return (day + 7) - want | |
| # Same week, simply walk back | |
| return day - want |
| def last_weekend(day: "DAYS_TYPE") -> "DAYS_TYPE": | ||
| """ | ||
| Args: | ||
| day: current day | ||
| Returns: | ||
| days until want | ||
| """ | ||
| if day in [5,6]: | ||
| return min(last_day(day-1, 5), last_day(day-1, 6)) - 1 | ||
| return last_day(day, 6) |
There was a problem hiding this comment.
last_weekend() off-by-one and negative offsets
Current logic may return negative numbers (e.g. ‑1 for Sunday) or an excessive offset (e.g. 4 from Wednesday → Saturday).
A safer implementation that always walks back to the most recent Sunday:
-def last_weekend(day: "DAYS_TYPE") -> "DAYS_TYPE":
- ...
- if day in [5,6]:
- return min(last_day(day-1, 5), last_day(day-1, 6)) - 1
- return last_day(day, 6)
+def last_weekend(day: "DAYS_TYPE") -> "DAYS_TYPE":
+ """
+ Days to the most recent Sunday (weekday 6).
+ Returns 0 when already on a weekend day.
+ """
+ if day == 6: # Sunday
+ return 0
+ if day == 5: # Saturday
+ return 1
+ return day + 1 # Monday-Friday📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| def last_weekend(day: "DAYS_TYPE") -> "DAYS_TYPE": | |
| """ | |
| Args: | |
| day: current day | |
| Returns: | |
| days until want | |
| """ | |
| if day in [5,6]: | |
| return min(last_day(day-1, 5), last_day(day-1, 6)) - 1 | |
| return last_day(day, 6) | |
| def last_weekend(day: "DAYS_TYPE") -> "DAYS_TYPE": | |
| """ | |
| Days to the most recent Sunday (weekday 6). | |
| Returns 0 when already on a weekend day. | |
| """ | |
| if day == 6: # Sunday | |
| return 0 | |
| if day == 5: # Saturday | |
| return 1 | |
| return day + 1 # Monday–Friday |
| def next_weekend(day: "DAYS_TYPE") -> "DAYS_TYPE": | ||
| """ | ||
| Args: | ||
| day: current day | ||
| Returns: | ||
| days until want | ||
| """ | ||
| if day in [5,6]: | ||
| return min(next_day(day+1, 5), next_day(day+1, 6)) + 1 | ||
| return next_day(day, 5) | ||
|
|
There was a problem hiding this comment.
next_weekend() can yield invalid weekday values (e.g. uses 7)
Passing day+1 to next_day() when day is already 6 produces 7, which is outside the Python weekday range 0-6 and causes logic errors.
A simpler, correct formulation:
-def next_weekend(day: "DAYS_TYPE") -> "DAYS_TYPE":
- ...
- if day in [5,6]:
- return min(next_day(day+1, 5), next_day(day+1, 6)) + 1
- return next_day(day, 5)
+def next_weekend(day: "DAYS_TYPE") -> "DAYS_TYPE":
+ """
+ Days until the upcoming Saturday (weekday 5).
+ Returns 0 when called on a weekend day.
+ """
+ if day <= 4: # Monday-Friday
+ return 5 - day
+ return 0 # Already weekend📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| def next_weekend(day: "DAYS_TYPE") -> "DAYS_TYPE": | |
| """ | |
| Args: | |
| day: current day | |
| Returns: | |
| days until want | |
| """ | |
| if day in [5,6]: | |
| return min(next_day(day+1, 5), next_day(day+1, 6)) + 1 | |
| return next_day(day, 5) | |
| def next_weekend(day: "DAYS_TYPE") -> "DAYS_TYPE": | |
| """ | |
| Days until the upcoming Saturday (weekday 5). | |
| Returns 0 when called on a weekend day. | |
| """ | |
| if day <= 4: # Monday–Friday | |
| return 5 - day | |
| return 0 # Already weekend |
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Actionable comments posted: 1
🔭 Outside diff range comments (1)
BetterHolidays/markets/nasdaq.py (1)
186-187:⚠️ Potential issueRemove debug print statements
These
- print(page) - print(len(page))
♻️ Duplicate comments (3)
BetterHolidays/markets/holidays.py (3)
50-60:⚠️ Potential issueFix
last_weekend()to handle edge cases correctlyThe current implementation has off-by-one errors and can return negative offsets, which might cause unexpected behavior when used in date calculations.
-def last_weekend(day: "DAYS_TYPE") -> "DAYS_TYPE": - """ - Args: - day: current day - - Returns: - days until want - """ - if day in [5, 6]: - return min(last_day(day - 1, 5), last_day(day - 1, 6)) - 1 - return last_day(day, 6) +def last_weekend(day: "DAYS_TYPE") -> "DAYS_TYPE": + """ + Days to the most recent Sunday (weekday 6). + Returns 0 when already on a weekend day. + + Args: + day: current day + + Returns: + days to previous weekend + """ + if day == 6: # Sunday + return 0 + if day == 5: # Saturday + return 1 + return day + 1 # Monday-Friday
25-34:⚠️ Potential issueFix
last_day()to actually go backwards in timeCurrently,
last_day()incorrectly delegates tonext_day(), which calculates in the wrong direction. This likely mis-calculates many holiday ranges, especially for the Chinese markets.-def last_day(day: "DAYS_TYPE", want: "DAYS_TYPE") -> "DAYS_TYPE": - """ - Args: - day: current day - want: day to get - - Returns: - days until want - """ - return next_day(day, want) +def last_day(day: "DAYS_TYPE", want: "DAYS_TYPE") -> "DAYS_TYPE": + """ + Days to step *backwards* from ``day`` to the previous ``want``. + Always non-negative so it can safely be used in ``datetime.timedelta``. + + Args: + day: current day + want: day to get + + Returns: + days to previous occurrence of want + """ + if day == want: + return 0 + if want > day: + # Wrap backwards over the week boundary + return (day + 7) - want + # Same week, simply walk back + return day - want
37-47:⚠️ Potential issueFix
next_weekend()to handle edge cases correctlyThe current implementation can yield invalid weekday values (e.g., day
7when day is already6), causing logic errors.-def next_weekend(day: "DAYS_TYPE") -> "DAYS_TYPE": - """ - Args: - day: current day - - Returns: - days until want - """ - if day in [5, 6]: - return min(next_day(day + 1, 5), next_day(day + 1, 6)) + 1 - return next_day(day, 5) +def next_weekend(day: "DAYS_TYPE") -> "DAYS_TYPE": + """ + Days until the upcoming Saturday (weekday 5). + Returns 0 when called on a weekend day. + + Args: + day: current day + + Returns: + days until weekend + """ + if day <= 4: # Monday-Friday + return 5 - day + return 0 # Already weekend
🧹 Nitpick comments (12)
BetterHolidays/utils.py (1)
87-97: Replace ambiguous variable name 'l'Using 'l' as a variable name is ambiguous as it can be confused with the number '1' in some fonts.
-def flatten_list(l: "list[list[T]|T1]", recursive: bool = False) -> "list[T1|T]": +def flatten_list(lst: "list[list[T]|T1]", recursive: bool = False) -> "list[T1|T]": ret = [] - for item in l: + for item in lst: if isinstance(item, list): if recursive: ret.extend(flatten_list(item, recursive)) else: ret.extend(item) else: ret.append(item) return ret🧰 Tools
🪛 Ruff (0.8.2)
87-87: Ambiguous variable name:
l(E741)
BetterHolidays/markets/lse.py (1)
53-54: Improve exception handling with "from" clauseCurrent exception handling obscures the original error's traceback. Use
from eto preserve it orfrom Noneto suppress it.- raise ValueError(f"Error fetching LSE data for {year}: {e}") + raise ValueError(f"Error fetching LSE data for {year}: {e}") from e🧰 Tools
🪛 Ruff (0.8.2)
54-54: Within an
exceptclause, raise exceptions withraise ... from errorraise ... from Noneto distinguish them from errors in exception handling(B904)
BetterHolidays/markets/cache.py (2)
17-19: Remove debug print statementThe
print(selection)statement appears to be debugging code that should be removed before production deployment.def get(self, key: "dt.date") -> "Day": selection = Day.select().where(Day.date == key, Day.market == self.market) - print(selection) return selection.get()
33-34: Improve exception handling with "from" clauseUse the
from Nonesyntax to clarify that this is a new exception rather than a chained one.- raise ValueError(f"Could not find day {key} after fetching data") + raise ValueError(f"Could not find day {key} after fetching data") from None🧰 Tools
🪛 Ruff (0.8.2)
34-34: Within an
exceptclause, raise exceptions withraise ... from errorraise ... from Noneto distinguish them from errors in exception handling(B904)
BetterHolidays/days.py (1)
39-39: Consider moving table creation to an initialization functionCreating tables directly at module level could cause issues if the module is imported multiple times or if circular imports exist. Consider moving this to an initialization function.
-utils.get_db().create_tables([Day, Holiday, TradingDay, NonTradingDay, PartialTradingDay]) +def initialize_tables(): + utils.get_db().create_tables([Day, Holiday, TradingDay, NonTradingDay, PartialTradingDay]) + +# Call the function at import time if needed +initialize_tables()BetterHolidays/markets/nasdaq.py (1)
106-107: Remove unnecessary f-string prefixThe URL string does not contain any placeholders, so the
fprefix is unnecessary.- page = md.HTML.from_url( - f"https://www.nasdaq.com/market-activity/stock-market-holiday-schedule" - ) + page = md.HTML.from_url( + "https://www.nasdaq.com/market-activity/stock-market-holiday-schedule" + )🧰 Tools
🪛 Ruff (0.8.2)
106-106: f-string without any placeholders
Remove extraneous
fprefix(F541)
BetterHolidays/markets/market.py (3)
152-156: Simplify boolean return statementsThe function can be simplified by directly returning the result of the
isinstancecheck.- @classmethod - def is_holiday(cls, date: "dt.date") -> bool: - day = cls.day(date) - if not isinstance(day, Holiday): - return False - return True + @classmethod + def is_holiday(cls, date: "dt.date") -> bool: + day = cls.day(date) + return isinstance(day, Holiday)🧰 Tools
🪛 Ruff (0.8.2)
154-156: Return the condition
isinstance(day, Holiday)directlyReplace with
return isinstance(day, Holiday)(SIM103)
159-163: Simplify boolean return statementsThe function can be simplified by directly returning the result of the
isinstancecheck.- @classmethod - def is_partial_day(cls, date: "dt.date") -> "bool": - day = cls.day(date) - if not isinstance(day, PartialTradingDay): - return False - return True + @classmethod + def is_partial_day(cls, date: "dt.date") -> "bool": + day = cls.day(date) + return isinstance(day, PartialTradingDay)🧰 Tools
🪛 Ruff (0.8.2)
161-163: Return the condition
isinstance(day, PartialTradingDay)directlyReplace with
return isinstance(day, PartialTradingDay)(SIM103)
166-170: Simplify boolean return statementsThe function can be simplified by directly returning the result of the
isinstancecheck.- @classmethod - def is_trading_day(cls, date: "dt.date") -> "bool": - day = cls.day(date) - if not isinstance(day, TradingDay): - return False - return True + @classmethod + def is_trading_day(cls, date: "dt.date") -> "bool": + day = cls.day(date) + return isinstance(day, TradingDay)🧰 Tools
🪛 Ruff (0.8.2)
168-170: Return the condition
isinstance(day, TradingDay)directlyReplace with
return isinstance(day, TradingDay)(SIM103)
BetterHolidays/markets/holidays.py (3)
353-355: Improve exception handling with proper error chainingUse
raise ... from esyntax to preserve the exception chain for better debugging.- except Exception as e: - raise ValueError(f"Error finding Good Friday information: {str(e)}") + except Exception as e: + raise ValueError(f"Error finding Good Friday information: {str(e)}") from e🧰 Tools
🪛 Ruff (0.8.2)
354-354: Within an
exceptclause, raise exceptions withraise ... from errorraise ... from Noneto distinguish them from errors in exception handling(B904)
360-362: Improve exception handling with proper error chainingUse
raise ... from esyntax to preserve the exception chain for better debugging.- raise ValueError( - f"Error determining Good Friday date for {year}: {str(e)} ({type(e)})" - ) + raise ValueError( + f"Error determining Good Friday date for {year}: {str(e)} ({type(e)})" + ) from e🧰 Tools
🪛 Ruff (0.8.2)
360-362: Within an
exceptclause, raise exceptions withraise ... from errorraise ... from Noneto distinguish them from errors in exception handling(B904)
3-3: Remove unused importThe
defaultfunction is imported but never used in this file.-from ..utils import flatten_list, default +from ..utils import flatten_list🧰 Tools
🪛 Ruff (0.8.2)
3-3:
..utils.defaultimported but unusedRemove unused import:
..utils.default(F401)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (11)
BetterHolidays/days.py(1 hunks)BetterHolidays/markets/__init__.py(1 hunks)BetterHolidays/markets/cache.py(1 hunks)BetterHolidays/markets/holidays.py(3 hunks)BetterHolidays/markets/lse.py(1 hunks)BetterHolidays/markets/market.py(2 hunks)BetterHolidays/markets/nasdaq.py(1 hunks)BetterHolidays/markets/nyse.py(1 hunks)BetterHolidays/markets/sse.py(1 hunks)BetterHolidays/utils.py(2 hunks)test.py(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (4)
- test.py
- BetterHolidays/markets/init.py
- BetterHolidays/markets/sse.py
- BetterHolidays/markets/nyse.py
🧰 Additional context used
🧬 Code Graph Analysis (3)
BetterHolidays/markets/cache.py (3)
BetterHolidays/utils.py (1)
get_db(40-41)BetterHolidays/days.py (1)
Day(4-11)BetterHolidays/markets/market.py (1)
day(180-181)
BetterHolidays/days.py (1)
BetterHolidays/utils.py (1)
get_db(40-41)
BetterHolidays/utils.py (1)
BetterHolidays/markets/market.py (1)
days(184-190)
🪛 Ruff (0.8.2)
BetterHolidays/markets/lse.py
54-54: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
BetterHolidays/markets/cache.py
34-34: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
BetterHolidays/markets/nasdaq.py
106-106: f-string without any placeholders
Remove extraneous f prefix
(F541)
BetterHolidays/markets/market.py
154-156: Return the condition isinstance(day, Holiday) directly
Replace with return isinstance(day, Holiday)
(SIM103)
161-163: Return the condition isinstance(day, PartialTradingDay) directly
Replace with return isinstance(day, PartialTradingDay)
(SIM103)
168-170: Return the condition isinstance(day, TradingDay) directly
Replace with return isinstance(day, TradingDay)
(SIM103)
BetterHolidays/markets/holidays.py
3-3: ..utils.default imported but unused
Remove unused import: ..utils.default
(F401)
98-99: Use a single if statement instead of nested if statements
(SIM102)
354-354: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
360-362: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
BetterHolidays/utils.py
87-87: Ambiguous variable name: l
(E741)
🔇 Additional comments (9)
BetterHolidays/utils.py (3)
17-18: Good fix for cache directory handlingYou've correctly fixed the typo in the cache directory name and added directory creation, addressing previous review comments effectively.
34-41: Well-implemented database connection setupThe implementation follows previous review suggestions to ensure the database is properly connected with
reuse_if_open=True. This avoids potential race conditions in multi-threaded contexts.
81-84: Good utility function for year iterationThe
iter_yearfunction provides a clean, reusable way to iterate through all dates in a given year, which is useful for market day processing.BetterHolidays/markets/lse.py (2)
1-7: Imports include classproperty as suggestedThe code now correctly imports
classpropertyfrom utils, addressing the previous review comment.
29-53: Well-structured market data fetching implementationThe implementation cleanly scrapes holiday data from an external calendar website, parses the dates, and properly classifies each day of the year as a Holiday, TradingDay, or NonTradingDay.
BetterHolidays/markets/cache.py (1)
21-23: Fixed property assignment before saveYou've properly fixed the issue by directly setting the
marketproperty before saving, as recommended in the previous review.BetterHolidays/days.py (2)
7-8: Database connection now works correctlyYour implementation of
get_db()with the@method_cachedecorator now correctly handles the database connection, addressing the previous review comment about missing arguments.
31-37: Well-designed PartialTradingDay modelThe PartialTradingDay model provides a clean way to represent days with special trading hours, including appropriate fields for early close and late open scenarios with reasons.
BetterHolidays/markets/holidays.py (1)
205-205:⚠️ Potential issueFix undefined name in type annotation
The type annotation uses
datewhich is undefined and could cause errors when annotations are evaluated.- def get_date(self, year: int) -> 'dt.date | list[dt.date]': + def get_date(self, year: int) -> 't.Union[dt.date, list[dt.date]]':Likely an incorrect or invalid review comment.
| def __contains__(self, key: "dt.date") -> bool: | ||
| return ( | ||
| key in Day.select().where(Day.market == self.market, Day.date == key).get() | ||
| ) |
There was a problem hiding this comment.
Fix incorrect implementation of contains
The current implementation tries to check if a date is within a Day object, which is incorrect. It should check if the query returns any results.
def __contains__(self, key: "dt.date") -> bool:
- return (
- key in Day.select().where(Day.market == self.market, Day.date == key).get()
- )
+ try:
+ Day.select().where(Day.market == self.market, Day.date == key).get()
+ return True
+ except pw.DoesNotExist:
+ return False📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| def __contains__(self, key: "dt.date") -> bool: | |
| return ( | |
| key in Day.select().where(Day.market == self.market, Day.date == key).get() | |
| ) | |
| def __contains__(self, key: "dt.date") -> bool: | |
| try: | |
| Day.select().where(Day.market == self.market, Day.date == key).get() | |
| return True | |
| except pw.DoesNotExist: | |
| return False |
1 participant
Summary by CodeRabbit
New Features
Improvements
Bug Fixes
Chores
platformdirsfor cache directory management.Tests