v0.1.1 #2
v0.1.1 #2
Conversation
WalkthroughThis update introduces several structural and functional changes across the codebase. The Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant BetterHolidays
participant multi
participant Market
participant Cache
User->>BetterHolidays: import get_market
BetterHolidays->>multi: get_market(name)
multi-->>BetterHolidays: Market class
BetterHolidays-->>User: Market class
User->>Market: is_holiday(date)
Market->>Cache: get_or_set(date, fetch_data)
Cache-->>Market: Day instance
Market-->>User: bool (is holiday)
User->>Market: get_holidays(start, end)
loop for each day in range
Market->>Cache: get_or_set(day, fetch_data)
Cache-->>Market: Day instance
end
Market-->>User: list of Holiday
Possibly related PRs
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (3)
✅ Files skipped from review due to trivial changes (2)
🚧 Files skipped from review as they are similar to previous changes (1)
✨ Finishing Touches
🪧 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: 11
🧹 Nitpick comments (14)
BetterHolidays/typing.py (1)
1-6: Looks good, but consider renamingClassMethodThe type definitions are clear and well-structured. However, the name
ClassMethodcould be misleading since it doesn't actually represent Python's class methods (which are decorated with@classmethod). It's a generic callable that takes one parameter and returns a value.Consider renaming this to something more descriptive like
SingleParamFunctionorSimpleCallableto better reflect its purpose:-ClassMethod = t.Callable[[T], T1] +SingleParamFunction = t.Callable[[T], T1]BetterHolidays/markets/cache.py (2)
6-7: Duplicate type variable definitionThe type variable
Tis already defined in thetyping.pymodule, which could be imported instead of redefined here.Consider importing
Tfrom the typing module:import datetime as dt import typing as t from ..days import Day from ..utils import NOT_SET +from ..typing import T -T = t.TypeVar("T")
8-11: Add class-level documentationThe
Cacheclass lacks documentation explaining its purpose and usage pattern.Add a class docstring:
class Cache: + """A cache for Day objects keyed by date. + + This class provides methods to manage a dictionary-based cache that maps + datetime.date objects to Day instances, supporting efficient date-to-day lookups. + """ def __init__(self): self.cache: 'dict[dt.date, Day]' = {}README.md (2)
13-26: Enhance code example with expected outputsThe code example is clear and demonstrates the main functionality well. However, it would be more educational to include the expected outputs of each print statement.
Consider adding comments showing expected outputs:
import BetterHolidays as bh import datetime as dt NYSE = bh.get_market("NYSE") # or NYSE = bh.NYSE() print(NYSE.is_holiday(dt.date(1979, 4, 13))) +# Output: True # Good Friday 1979 print(NYSE.is_trading_day(dt.date(1979, 4, 13))) +# Output: False print(NYSE.is_partial_day(dt.date(1979, 4, 13))) +# Output: False print(NYSE.is_weekday(dt.date(1979, 4, 13))) +# Output: True print(NYSE.is_weekend(dt.date(1979, 4, 13))) +# Output: False print(NYSE.get_holidays(dt.date(1979, 4, 1), dt.date(1979, 4, 30))) +# Output: [Holiday(1979-04-13, 'Good Friday')]
28-30: Expand the Contributing sectionThe current Contributing section is minimal and could benefit from more detailed guidelines.
Consider expanding the Contributing section:
## Contributing -Pull requests are welcome. +Pull requests are welcome. For major changes, please open an issue first to discuss what you would like to change. + +Please make sure to update tests as appropriate. + +### Development Setup + +```bash +# Clone the repository +git clone https://github.com/yourusername/better-holidays.git +cd better-holidays + +# Install development dependencies +pip install -e ".[dev]" + +# Run tests +pytest +```BetterHolidays/markets/nyse.py (1)
60-60: Remove stray comment character.There's a stray
#character at the end of the line that should be removed.- dt.date(2006, 12, 27): PartialTradingDay(name="President Gerald Ford's death", date=dt.date(2006, 12, 27), open_time=dt.time(hour=9, minute=32), close_time=standard_close_time, late_open=True, late_open_reason="Moment of silence for President Gerald Ford's death"),# + dt.date(2006, 12, 27): PartialTradingDay(name="President Gerald Ford's death", date=dt.date(2006, 12, 27), open_time=dt.time(hour=9, minute=32), close_time=standard_close_time, late_open=True, late_open_reason="Moment of silence for President Gerald Ford's death"),BetterHolidays/markets/holidays.py (1)
129-129: Fix unnecessary f-string.This string doesn't contain any format variables, so the
fprefix is unnecessary.- elements = html.inner_html.advanced_find("a", attrs={"href": f"https://www.calendar-365.co.uk/holidays/good-friday.html", "class": "link_arrow", "title": "Good Friday 2026", "text": "Good Friday"}) + elements = html.inner_html.advanced_find("a", attrs={"href": "https://www.calendar-365.co.uk/holidays/good-friday.html", "class": "link_arrow", "title": "Good Friday 2026", "text": "Good Friday"})🧰 Tools
🪛 Ruff (0.8.2)
129-129: f-string without any placeholders
Remove extraneous
fprefix(F541)
BetterHolidays/utils.py (2)
23-30: Use identity comparison for the NOT_SET sentinel
NOT_SETis a unique sentinel object. Comparing it with==works becausetypeobjects fall back to identity comparison, but it is clearer (and marginally faster) to useis/is not. This eliminates any risk if==were ever overloaded on a different sentinel implementation.- if default == NOT_SET: + if default is NOT_SET:
68-72: Minor: name the generator parameter for clarityA tiny readability win –
current_datebetter communicates the loop variable.- current = start - while current <= end: - yield current - current += dt.timedelta(days=1) + current_date = start + while current_date <= end: + yield current_date + current_date += dt.timedelta(days=1)BetterHolidays/markets/market.py (5)
21-27:validate_optionsnever runs – call it in__init_subclass__You defined robust validation but never invoke it. Running it when a subclass
is created gives immediate feedback to library users.- def __init_subclass__(cls) -> None: - cls.cache = Cache() + def __init_subclass__(cls) -> None: + cls.cache = Cache() + cls.validate_options()
57-69: Repeated date iteration is O(N) – consider year-level pre-filtering
get_*helpers iterate every single date betweenstartandend, then look
each day up in the cache. For multi-year ranges this becomes expensive
(~365 × years look-ups).If the underlying
fetch_dataalready loads all days for a given year, expose
a year-range API instead and filter in memory once per year.
71-97: Simplifyis_*helpers – return the boolean expression directlyRuff already flagged this. Returning the expression shortens the code and
avoids an unnecessary variable.- def is_holiday(cls, date: 'dt.date') -> bool: - day = cls.day(date) - if not isinstance(day, Holiday): - return False - return True + def is_holiday(cls, date: 'dt.date') -> bool: + return isinstance(cls.day(date), Holiday) - def is_partial_day(cls, date: 'dt.date') -> 'bool': - day = cls.day(date) - if not isinstance(day, PartialTradingDay): - return False - return True + def is_partial_day(cls, date: 'dt.date') -> bool: + return isinstance(cls.day(date), PartialTradingDay) - def is_trading_day(cls, date: 'dt.date') -> 'bool': - day = cls.day(date) - if not isinstance(day, TradingDay): - return False - return True + def is_trading_day(cls, date: 'dt.date') -> 'bool': + return isinstance(cls.day(date), TradingDay)🧰 Tools
🪛 Ruff (0.8.2)
81-83: Return the condition
isinstance(day, Holiday)directlyReplace with
return isinstance(day, Holiday)(SIM103)
88-90: Return the condition
isinstance(day, PartialTradingDay)directlyReplace with
return isinstance(day, PartialTradingDay)(SIM103)
95-97: Return the condition
isinstance(day, TradingDay)directlyReplace with
return isinstance(day, TradingDay)(SIM103)
100-105: Return type isTradingDay | None– update the annotation
get_trading_dayadvertisest.Optional[TradingDay]but the code returns
Nonewhen the date is not a trading day. Good! Just a reminder that using
|syntax (TradingDay | None) would read better on Python ≥3.10.
107-108: Potential race condition in cache population
Cache.get_or_setcallscls.fetch_datawhen the year key is missing.
If two threads request dates from the same untouched year simultaneously,
both could invokefetch_data, wasting work or causing duplicate writes.Consider adding a per-year lock inside
Cache.get_or_setor guarding the
fetch_datacall inMarket.day.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (10)
BetterHolidays/__init__.py(1 hunks)BetterHolidays/days.py(2 hunks)BetterHolidays/markets/cache.py(1 hunks)BetterHolidays/markets/holidays.py(2 hunks)BetterHolidays/markets/market.py(4 hunks)BetterHolidays/markets/nyse.py(3 hunks)BetterHolidays/typing.py(1 hunks)BetterHolidays/utils.py(1 hunks)README.md(1 hunks)requirements.txt(1 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (4)
BetterHolidays/__init__.py (1)
BetterHolidays/utils.py (3)
get_market(18-18)get_market(21-21)get_market(23-30)
BetterHolidays/markets/nyse.py (1)
BetterHolidays/days.py (2)
PartialTradingDay(26-32)Holiday(10-12)
BetterHolidays/markets/cache.py (1)
BetterHolidays/days.py (1)
Day(5-7)
BetterHolidays/markets/market.py (4)
BetterHolidays/utils.py (2)
iterate_date(68-72)classproperty(41-62)BetterHolidays/markets/cache.py (1)
Cache(8-42)BetterHolidays/markets/nyse.py (1)
weekdays(92-93)BetterHolidays/days.py (3)
Holiday(10-12)PartialTradingDay(26-32)TradingDay(15-18)
🪛 Ruff (0.8.2)
BetterHolidays/markets/holidays.py
129-129: f-string without any placeholders
Remove extraneous f prefix
(F541)
133-133: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
139-139: 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/market.py
81-83: Return the condition isinstance(day, Holiday) directly
Replace with return isinstance(day, Holiday)
(SIM103)
88-90: Return the condition isinstance(day, PartialTradingDay) directly
Replace with return isinstance(day, PartialTradingDay)
(SIM103)
95-97: Return the condition isinstance(day, TradingDay) directly
Replace with return isinstance(day, TradingDay)
(SIM103)
🔇 Additional comments (8)
BetterHolidays/__init__.py (2)
3-3: Excellent refactoring: Movedget_marketto a dedicated utility module.Moving the
get_marketfunction to a centralized utility module follows good software design principles by reducing code duplication and improving maintainability.
14-14:__all__list properly updated to includeget_market.The function is correctly exposed in the package's public API.
BetterHolidays/days.py (2)
12-12: Good change: Makenamea required parameter forHolidayclass.Removing the default empty string value for the
nameattribute forces explicit naming of all holidays, which improves code clarity and prevents potential errors from unnamed holidays.
28-28: Good consistency: Explicitly declaringnameinPartialTradingDay.Although
PartialTradingDayinherits fromHolidaywhich already defines thenamefield, explicitly declaring it here makes the code more self-documenting and reinforces that this is a required attribute.BetterHolidays/markets/nyse.py (1)
156-156: Good addition: Include name parameter in dynamically created PartialTradingDay instances.Adding the name parameter to dynamically created PartialTradingDay instances aligns with the changes in the
days.pymodule and provides better identification of partial trading days.BetterHolidays/markets/holidays.py (2)
5-5: Good addition: Import re module for regex operations.The import supports the new regex-based date parsing in the GoodFriday class.
118-118: Good practice: Compile regex pattern as class attribute.Compiling the regex pattern as a class attribute is more efficient than compiling it each time the method is called.
BetterHolidays/markets/market.py (1)
42-45: Thread-safety of_weekendscaching
_weekendsis lazily initialised without any synchronisation. In a multi-threaded
context two threads could compute the list simultaneously, with one result lost.
Ifweekendsis performance-critical, guard the assignment with a lock or
pre-compute at class creation time.
| def get_or_set(self, key: 'dt.date', func: 't.Callable[[int], None]') -> 'Day': | ||
| if key in self.cache: | ||
| return self.get(key) | ||
| func(key.year) | ||
| if key in self.cache: | ||
| return self.get(key) | ||
| raise ValueError("Cache miss") |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Improve get_or_set signature and documentation
The get_or_set method has a potentially confusing type annotation. The function func is typed as t.Callable[[int], None], but it's expected to populate the cache as a side effect rather than returning a value. This implicit behavior isn't clear from the signature.
Add docstring and improve the type annotation:
- def get_or_set(self, key: 'dt.date', func: 't.Callable[[int], None]') -> 'Day':
+ def get_or_set(self, key: 'dt.date', func: 't.Callable[[int], None]') -> 'Day':
+ """Get a Day from cache or populate cache by calling func with the year.
+
+ Args:
+ key: The date to look up
+ func: A function that takes a year (int) and populates the cache as a side effect
+
+ Returns:
+ The cached Day object
+
+ Raises:
+ ValueError: If the cache still doesn't contain the key after calling func
+ """
if key in self.cache:
return self.get(key)
func(key.year)
if key in self.cache:
return self.get(key)
raise ValueError("Cache miss")📝 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 get_or_set(self, key: 'dt.date', func: 't.Callable[[int], None]') -> 'Day': | |
| if key in self.cache: | |
| return self.get(key) | |
| func(key.year) | |
| if key in self.cache: | |
| return self.get(key) | |
| raise ValueError("Cache miss") | |
| def get_or_set(self, key: 'dt.date', func: 't.Callable[[int], None]') -> 'Day': | |
| """Get a Day from cache or populate cache by calling func with the year. | |
| Args: | |
| key: The date to look up | |
| func: A function that takes a year (int) and populates the cache as a side effect | |
| Returns: | |
| The cached Day object | |
| Raises: | |
| ValueError: If the cache still doesn't contain the key after calling func | |
| """ | |
| if key in self.cache: | |
| return self.get(key) | |
| func(key.year) | |
| if key in self.cache: | |
| return self.get(key) | |
| raise ValueError("Cache miss") |
| @@ -0,0 +1 @@ | |||
| better-md>=0.3.4 | |||
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Verify better-md dependency usage
Adding the better-md dependency for HTML fetching and parsing is appropriate for the GoodFriday holiday logic.
🏁 Script executed:
#!/bin/bash
# Check how better-md is being used in the codebase
echo "Searching for import statements of better-md:"
rg -l "import better[_-]md|from better[_-]md"
echo -e "\nChecking usage contexts:"
rg -A 3 -B 3 "better[_-]md" Length of output: 633
Ensure better-md Is Actually Used or Remove It
I couldn’t find any imports or calls to better-md in the codebase—its only occurrences are in requirements.txt and setup.py. If you added this dependency to fetch and parse HTML for the GoodFriday logic, please either:
-
Import and use
better_md(orbetter-md) in the module responsible for GoodFriday holiday parsing, for example:from better_md import fetch_html, parse_markdown
-
Or, if the feature hasn’t been implemented yet, remove
better-md>=0.3.4from bothrequirements.txtandsetup.pyto avoid shipping an unused dependency.
| if not elements: | ||
| raise ValueError(f"Could not find Good Friday information for {year}") | ||
| except Exception as e: | ||
| raise ValueError(f"Error finding Good Friday information: {str(e)}") |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Use proper exception chaining.
When raising a new exception from within an except block, use from e to maintain the exception chain for better debugging.
- raise ValueError(f"Error finding Good Friday information: {str(e)}")
+ raise ValueError(f"Error finding Good Friday information: {str(e)}") from e📝 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.
| raise ValueError(f"Error finding Good Friday information: {str(e)}") | |
| raise ValueError(f"Error finding Good Friday information: {str(e)}") from e |
🧰 Tools
🪛 Ruff (0.8.2)
133-133: 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/holidays.py
Outdated
| def get_date(self, year: 'int') -> 'dt.date': | ||
| try: | ||
| url = f"https://www.officesimplify.com/bank-holidays-uk-{year}" | ||
| html = md.HTML.from_url(url) | ||
| elements = html.inner_html.advanced_find("td", attrs={"text": "Good Friday"}) | ||
| if not elements: | ||
| raise ValueError(f"Could not find Good Friday information for {year}") | ||
| tr = elements[0].parent | ||
| day, month = tr.children[0].text.split(" ") | ||
| return dt.date(year, MONTHS_MAP[month], int(day[:-2])) | ||
| except Exception as e: | ||
| raise ValueError(f"Error determining Good Friday date for {year}: {str(e)}") | ||
| url = f"https://www.calendar-365.co.uk/holidays/{year}.html" | ||
| try: | ||
| html = md.HTML.from_url(url) | ||
| except Exception as e: | ||
| raise ValueError(f"Better Markdown error: {str(e)}") from e | ||
|
|
||
| try: | ||
| elements = html.inner_html.advanced_find("a", attrs={"href": f"https://www.calendar-365.co.uk/holidays/good-friday.html", "class": "link_arrow", "title": "Good Friday 2026", "text": "Good Friday"}) | ||
| if not elements: | ||
| raise ValueError(f"Could not find Good Friday information for {year}") | ||
| except Exception as e: | ||
| raise ValueError(f"Error finding Good Friday information: {str(e)}") | ||
|
|
||
| tr = elements[0].parent.parent | ||
| day, month, _ = self.regex.match(tr.children[0].text).groups() | ||
| 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)})") | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Improved implementation for GoodFriday.get_date method.
The new implementation uses a more reliable data source and includes comprehensive error handling. A few improvements can be made:
- Remove unnecessary f-string that doesn't use any variables
- Use proper exception chaining with
from esyntax
def get_date(self, year: 'int') -> 'dt.date':
try:
url = f"https://www.calendar-365.co.uk/holidays/{year}.html"
try:
html = md.HTML.from_url(url)
except Exception as e:
raise ValueError(f"Better Markdown error: {str(e)}") from e
try:
- elements = html.inner_html.advanced_find("a", attrs={"href": f"https://www.calendar-365.co.uk/holidays/good-friday.html", "class": "link_arrow", "title": "Good Friday 2026", "text": "Good Friday"})
+ elements = html.inner_html.advanced_find("a", attrs={"href": "https://www.calendar-365.co.uk/holidays/good-friday.html", "class": "link_arrow", "title": "Good Friday 2026", "text": "Good Friday"})
if not elements:
raise ValueError(f"Could not find Good Friday information for {year}")
except Exception as e:
- raise ValueError(f"Error finding Good Friday information: {str(e)}")
+ raise ValueError(f"Error finding Good Friday information: {str(e)}") from e
tr = elements[0].parent.parent
day, month, _ = self.regex.match(tr.children[0].text).groups()
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)})")
+ raise ValueError(f"Error determining Good Friday date for {year}: {str(e)} ({type(e)})") from e📝 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 get_date(self, year: 'int') -> 'dt.date': | |
| try: | |
| url = f"https://www.officesimplify.com/bank-holidays-uk-{year}" | |
| html = md.HTML.from_url(url) | |
| elements = html.inner_html.advanced_find("td", attrs={"text": "Good Friday"}) | |
| if not elements: | |
| raise ValueError(f"Could not find Good Friday information for {year}") | |
| tr = elements[0].parent | |
| day, month = tr.children[0].text.split(" ") | |
| return dt.date(year, MONTHS_MAP[month], int(day[:-2])) | |
| except Exception as e: | |
| raise ValueError(f"Error determining Good Friday date for {year}: {str(e)}") | |
| url = f"https://www.calendar-365.co.uk/holidays/{year}.html" | |
| try: | |
| html = md.HTML.from_url(url) | |
| except Exception as e: | |
| raise ValueError(f"Better Markdown error: {str(e)}") from e | |
| try: | |
| elements = html.inner_html.advanced_find("a", attrs={"href": f"https://www.calendar-365.co.uk/holidays/good-friday.html", "class": "link_arrow", "title": "Good Friday 2026", "text": "Good Friday"}) | |
| if not elements: | |
| raise ValueError(f"Could not find Good Friday information for {year}") | |
| except Exception as e: | |
| raise ValueError(f"Error finding Good Friday information: {str(e)}") | |
| tr = elements[0].parent.parent | |
| day, month, _ = self.regex.match(tr.children[0].text).groups() | |
| 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)})") | |
| def get_date(self, year: 'int') -> 'dt.date': | |
| try: | |
| url = f"https://www.calendar-365.co.uk/holidays/{year}.html" | |
| try: | |
| html = md.HTML.from_url(url) | |
| except Exception as e: | |
| raise ValueError(f"Better Markdown error: {str(e)}") from e | |
| try: | |
| elements = html.inner_html.advanced_find( | |
| "a", | |
| attrs={ | |
| "href": "https://www.calendar-365.co.uk/holidays/good-friday.html", | |
| "class": "link_arrow", | |
| "title": "Good Friday 2026", | |
| "text": "Good Friday", | |
| }, | |
| ) | |
| if not elements: | |
| raise ValueError(f"Could not find Good Friday information for {year}") | |
| except Exception as e: | |
| raise ValueError(f"Error finding Good Friday information: {str(e)}") from e | |
| tr = elements[0].parent.parent | |
| day, month, _ = self.regex.match(tr.children[0].text).groups() | |
| 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)})" | |
| ) from e |
🧰 Tools
🪛 Ruff (0.8.2)
129-129: f-string without any placeholders
Remove extraneous f prefix
(F541)
133-133: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
139-139: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
| def method_cache(cache_method: t.Callable[[], T1]): | ||
| def wrapper(func: t.Callable[[T], T1]): | ||
| cache = cache_method() | ||
| @ft.wraps(func) | ||
| def inner(*args, **kwargs): | ||
| return func(*args, **kwargs, cache=cache) | ||
| return inner | ||
| return wrapper |
There was a problem hiding this comment.
🛠️ Refactor suggestion
method_cache forces an unexpected “cache” kw-arg onto every decorated function
The decorator always calls the wrapped function as func(*args, **kwargs, cache=cache).
That means:
- The original function must accept a
cachekeyword or it will raiseTypeError. - Type hints (
Callable[[T], T1]) are now misleading because the real signature is different. - Users of the decorated function have no idea a hidden kw-arg is being injected.
Consider rewriting so that the cache is closed over instead of injected, e.g.:
- def inner(*args, **kwargs):
- return func(*args, **kwargs, cache=cache)
+ def inner(*args, **kwargs):
+ return func(*args, **kwargs, _cache=cache)(or remove the extra parameter entirely and let the inner wrapper handle caching).
Committable suggestion skipped: line range outside the PR's diff.
| @method_cache(lambda: {"type": type("ABSTRACT_CONST", (object,), {"__isabstractmethod__": True})}) | ||
| def abstract_const(cache): | ||
| return cache["type"]() |
There was a problem hiding this comment.
abstract_const is not constant – a new instance is built on every call
Because cache["type"]() is executed each time, every invocation returns a
fresh object. If the goal is a singleton constant, memoise the instance:
-@method_cache(lambda: {"type": type("ABSTRACT_CONST", (object,), {"__isabstractmethod__": True})})
-def abstract_const(cache):
- return cache["type"]()
+@method_cache(
+ lambda: {
+ "type": type("ABSTRACT_CONST", (object,), {"__isabstractmethod__": True}),
+ "instance": None,
+ }
+)
+def abstract_const(cache):
+ if cache["instance"] is None:
+ cache["instance"] = cache["type"]()
+ return cache["instance"]📝 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.
| @method_cache(lambda: {"type": type("ABSTRACT_CONST", (object,), {"__isabstractmethod__": True})}) | |
| def abstract_const(cache): | |
| return cache["type"]() | |
| @method_cache( | |
| lambda: { | |
| "type": type("ABSTRACT_CONST", (object,), {"__isabstractmethod__": True}), | |
| "instance": None, | |
| } | |
| ) | |
| def abstract_const(cache): | |
| if cache["instance"] is None: | |
| cache["instance"] = cache["type"]() | |
| return cache["instance"] |
There was a problem hiding this comment.
This function says that a constant variable has to be defined in the subclass.
There was a problem hiding this comment.
@R5dan I see now - thank you for the clarification! The abstract_const function is intentionally creating a new instance with __isabstractmethod__ set to True to serve as a marker indicating that a constant needs to be defined/overridden in subclasses. This is similar to how abstract methods work in Python's ABC module.
My previous suggestion to memoize the instance wouldn't be appropriate here since the function is deliberately returning a fresh marker object each time. I appreciate you explaining the design intention.
✏️ Learnings added
Learnt from: R5dan
PR: Better-Python/Better-Holidays#2
File: BetterHolidays/utils.py:64-66
Timestamp: 2025-05-04T21:17:33.203Z
Learning: The `abstract_const` function in BetterHolidays/utils.py is designed to create marker objects that indicate a constant needs to be defined in a subclass. It intentionally returns a new instance with `__isabstractmethod__` set to `True` each time it's called, similar to how abstract methods work in Python's ABC module.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
| class classproperty(t.Generic[T, T1]): | ||
| def __init__(self, getter: ClassMethod[T, T1]): | ||
| self.getter = getter | ||
| self.setter = lambda val: not_set("set", self.getter.__name__) | ||
| self.deleter = lambda: not_set("delete", self.getter.__name__) | ||
|
|
||
| def set(self, method: 'ClassMethod[T, None]'): | ||
| self.setter = method | ||
| return self | ||
|
|
||
| def delete(self, method: 'ClassMethod[T, None]'): | ||
| self.deleter = method | ||
| return self | ||
|
|
||
| def __get__(self, instance, owner): | ||
| return self.getter(owner) | ||
|
|
||
| def __set__(self, instance, value): | ||
| self.setter(value) | ||
|
|
||
| def __delete__(self, instance): | ||
| self.deleter() | ||
|
|
There was a problem hiding this comment.
classproperty setter / deleter lose the cls argument
__set__ and __delete__ call self.setter(value) / self.deleter() but the
user-supplied methods declared via .set / .delete expect the class as
their first argument (ClassMethod[T, None]). At runtime this leads to
TypeError: <method>() missing 1 required positional argument: 'cls'.
- def __set__(self, instance, value):
- self.setter(value)
+ def __set__(self, instance, value):
+ owner = instance.__class__ if instance is not None else value.__class__
+ self.setter(owner, value)
- def __delete__(self, instance):
- self.deleter()
+ def __delete__(self, instance):
+ owner = instance.__class__ if instance is not None else type(instance)
+ self.deleter(owner)📝 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.
| class classproperty(t.Generic[T, T1]): | |
| def __init__(self, getter: ClassMethod[T, T1]): | |
| self.getter = getter | |
| self.setter = lambda val: not_set("set", self.getter.__name__) | |
| self.deleter = lambda: not_set("delete", self.getter.__name__) | |
| def set(self, method: 'ClassMethod[T, None]'): | |
| self.setter = method | |
| return self | |
| def delete(self, method: 'ClassMethod[T, None]'): | |
| self.deleter = method | |
| return self | |
| def __get__(self, instance, owner): | |
| return self.getter(owner) | |
| def __set__(self, instance, value): | |
| self.setter(value) | |
| def __delete__(self, instance): | |
| self.deleter() | |
| def __get__(self, instance, owner): | |
| return self.getter(owner) | |
| def __set__(self, instance, value): | |
| owner = instance.__class__ if instance is not None else value.__class__ | |
| self.setter(owner, value) | |
| def __delete__(self, instance): | |
| owner = instance.__class__ if instance is not None else type(instance) | |
| self.deleter(owner) |
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
BetterHolidays/markets/holidays.py (2)
133-133: Use proper exception chaining.When raising a new exception from within an except block, use
from eto maintain the exception chain for better debugging.- raise ValueError(f"Error finding Good Friday information: {str(e)}") + raise ValueError(f"Error finding Good Friday information: {str(e)}") from e🧰 Tools
🪛 Ruff (0.8.2)
133-133: Within an
exceptclause, raise exceptions withraise ... from errorraise ... from Noneto distinguish them from errors in exception handling(B904)
139-139: Use proper exception chaining.When raising a new exception from within an except block, use
from eto maintain 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)
139-139: Within an
exceptclause, raise exceptions withraise ... from errorraise ... from Noneto distinguish them from errors in exception handling(B904)
🧹 Nitpick comments (2)
BetterHolidays/markets/holidays.py (2)
129-129: Remove unnecessary f-string prefix.The string doesn't contain any variables to interpolate, so the f-string prefix is unnecessary.
- elements = html.inner_html.advanced_find("a", attrs={"href": "https://www.calendar-365.co.uk/holidays/good-friday.html", "class": "link_arrow", "title": "Good Friday 2026", "text": "Good Friday"}) # The title is 'Good Friday 2026' for all years + elements = html.inner_html.advanced_find("a", attrs={"href": "https://www.calendar-365.co.uk/holidays/good-friday.html", "class": "link_arrow", "title": "Good Friday 2026", "text": "Good Friday"}) # The title is 'Good Friday 2026' for all years
136-136: Consider error handling for regex matching.If the regex pattern doesn't match the text, it will raise an AttributeError. Consider adding explicit error handling for this case.
- day, month, _ = self.regex.match(tr.children[0].text).groups() + match = self.regex.match(tr.children[0].text) + if not match: + raise ValueError(f"Failed to parse date format from '{tr.children[0].text}'") + day, month, _ = match.groups()
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
BetterHolidays/markets/holidays.py(2 hunks)BetterHolidays/markets/nyse.py(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- BetterHolidays/markets/nyse.py
🧰 Additional context used
🪛 Ruff (0.8.2)
BetterHolidays/markets/holidays.py
133-133: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
139-139: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
🔇 Additional comments (3)
BetterHolidays/markets/holidays.py (3)
5-5: Import ofremodule looks good.The addition of the
remodule is appropriate for the new regex pattern used in the GoodFriday class.
118-118: Good pattern for date extraction.The compiled regex pattern is well-defined for extracting date components from strings in the format "DD MONTH YYYY".
129-129:✅ Verification successful
Potential brittleness with hardcoded title attribute.
The hardcoded title "Good Friday 2026" might cause problems if the website changes its format. Consider making this more dynamic based on the year parameter.
🏁 Script executed:
#!/bin/bash # Check if the website actually uses the same title for different years for year in 2023 2024 2025 2026; do echo "Checking Good Friday title for year $year" curl -s "https://www.calendar-365.co.uk/holidays/${year}.html" | grep -o "Good Friday [0-9]\{4\}" | head -1 doneLength of output: 987
No action needed: “Good Friday 2026” is consistently used
Verified that the holiday pages for 2023–2026 all report the title attribute as “Good Friday 2026”. Since the site currently hardcodes this value across years, the existing selector is correct and not brittle.
| def get_date(self, year: 'int') -> 'dt.date': | ||
| try: | ||
| url = f"https://www.officesimplify.com/bank-holidays-uk-{year}" | ||
| html = md.HTML.from_url(url) | ||
| elements = html.inner_html.advanced_find("td", attrs={"text": "Good Friday"}) | ||
| if not elements: | ||
| raise ValueError(f"Could not find Good Friday information for {year}") | ||
| tr = elements[0].parent | ||
| day, month = tr.children[0].text.split(" ") | ||
| return dt.date(year, MONTHS_MAP[month], int(day[:-2])) | ||
| except Exception as e: | ||
| raise ValueError(f"Error determining Good Friday date for {year}: {str(e)}") | ||
| url = f"https://www.calendar-365.co.uk/holidays/{year}.html" | ||
| try: | ||
| html = md.HTML.from_url(url) | ||
| except Exception as e: | ||
| raise ValueError(f"Better Markdown error: {str(e)}") from e | ||
|
|
||
| try: | ||
| elements = html.inner_html.advanced_find("a", attrs={"href": "https://www.calendar-365.co.uk/holidays/good-friday.html", "class": "link_arrow", "title": "Good Friday 2026", "text": "Good Friday"}) # The title is 'Good Friday 2026' for all years | ||
| if not elements: | ||
| raise ValueError(f"Could not find Good Friday information for {year}") | ||
| except Exception as e: | ||
| raise ValueError(f"Error finding Good Friday information: {str(e)}") | ||
|
|
||
| tr = elements[0].parent.parent | ||
| day, month, _ = self.regex.match(tr.children[0].text).groups() | ||
| 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)})") |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider adding a fallback mechanism.
The method relies on a specific website structure which could change or become unavailable. Consider implementing a fallback mechanism or caching previous results.
def get_date(self, year: 'int') -> 'dt.date':
try:
url = f"https://www.calendar-365.co.uk/holidays/{year}.html"
try:
html = md.HTML.from_url(url)
except Exception as e:
raise ValueError(f"Better Markdown error: {str(e)}") from e
try:
elements = html.inner_html.advanced_find("a", attrs={"href": "https://www.calendar-365.co.uk/holidays/good-friday.html", "class": "link_arrow", "title": "Good Friday 2026", "text": "Good Friday"}) # The title is 'Good Friday 2026' for all years
if not elements:
raise ValueError(f"Could not find Good Friday information for {year}")
except Exception as e:
raise ValueError(f"Error finding Good Friday information: {str(e)}") from e
tr = elements[0].parent.parent
day, month, _ = self.regex.match(tr.children[0].text).groups()
return dt.date(year, MONTHS_MAP[month.upper()], int(day))
+ except ValueError as ve:
+ # Try fallback calculation method
+ try:
+ # Easter is the first Sunday after the first full moon after the vernal equinox
+ # Good Friday is the Friday before Easter Sunday
+ # This is a simplified implementation that works for most years
+ a = year % 19
+ b = year // 100
+ c = year % 100
+ d = b // 4
+ e = b % 4
+ f = (b + 8) // 25
+ g = (b - f + 1) // 3
+ h = (19 * a + b - d - g + 15) % 30
+ i = c // 4
+ k = c % 4
+ l = (32 + 2 * e + 2 * i - h - k) % 7
+ m = (a + 11 * h + 22 * l) // 451
+ month = (h + l - 7 * m + 114) // 31
+ day = ((h + l - 7 * m + 114) % 31) + 1
+ easter = dt.date(year, month, day)
+ good_friday = easter - dt.timedelta(days=2)
+ return good_friday
except Exception as e:
raise ValueError(f"Error determining Good Friday date for {year}: {str(e)} ({type(e)})") from e📝 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 get_date(self, year: 'int') -> 'dt.date': | |
| try: | |
| url = f"https://www.officesimplify.com/bank-holidays-uk-{year}" | |
| html = md.HTML.from_url(url) | |
| elements = html.inner_html.advanced_find("td", attrs={"text": "Good Friday"}) | |
| if not elements: | |
| raise ValueError(f"Could not find Good Friday information for {year}") | |
| tr = elements[0].parent | |
| day, month = tr.children[0].text.split(" ") | |
| return dt.date(year, MONTHS_MAP[month], int(day[:-2])) | |
| except Exception as e: | |
| raise ValueError(f"Error determining Good Friday date for {year}: {str(e)}") | |
| url = f"https://www.calendar-365.co.uk/holidays/{year}.html" | |
| try: | |
| html = md.HTML.from_url(url) | |
| except Exception as e: | |
| raise ValueError(f"Better Markdown error: {str(e)}") from e | |
| try: | |
| elements = html.inner_html.advanced_find("a", attrs={"href": "https://www.calendar-365.co.uk/holidays/good-friday.html", "class": "link_arrow", "title": "Good Friday 2026", "text": "Good Friday"}) # The title is 'Good Friday 2026' for all years | |
| if not elements: | |
| raise ValueError(f"Could not find Good Friday information for {year}") | |
| except Exception as e: | |
| raise ValueError(f"Error finding Good Friday information: {str(e)}") | |
| tr = elements[0].parent.parent | |
| day, month, _ = self.regex.match(tr.children[0].text).groups() | |
| 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)})") | |
| def get_date(self, year: 'int') -> 'dt.date': | |
| try: | |
| url = f"https://www.calendar-365.co.uk/holidays/{year}.html" | |
| try: | |
| html = md.HTML.from_url(url) | |
| except Exception as e: | |
| raise ValueError(f"Better Markdown error: {str(e)}") from e | |
| try: | |
| elements = html.inner_html.advanced_find( | |
| "a", | |
| attrs={ | |
| "href": "https://www.calendar-365.co.uk/holidays/good-friday.html", | |
| "class": "link_arrow", | |
| "title": "Good Friday 2026", | |
| "text": "Good Friday", | |
| }, | |
| ) # The title is 'Good Friday 2026' for all years | |
| if not elements: | |
| raise ValueError(f"Could not find Good Friday information for {year}") | |
| except Exception as e: | |
| raise ValueError(f"Error finding Good Friday information: {str(e)}") from e | |
| tr = elements[0].parent.parent | |
| day, month, _ = self.regex.match(tr.children[0].text).groups() | |
| return dt.date(year, MONTHS_MAP[month.upper()], int(day)) | |
| except ValueError as ve: | |
| # Fallback: compute Good Friday via the Anonymous Gregorian algorithm | |
| try: | |
| a = year % 19 | |
| b = year // 100 | |
| c = year % 100 | |
| d = b // 4 | |
| e = b % 4 | |
| f = (b + 8) // 25 | |
| g = (b - f + 1) // 3 | |
| h = (19 * a + b - d - g + 15) % 30 | |
| i = c // 4 | |
| k = c % 4 | |
| l = (32 + 2 * e + 2 * i - h - k) % 7 | |
| m = (a + 11 * h + 22 * l) // 451 | |
| month_e = (h + l - 7 * m + 114) // 31 | |
| day_e = ((h + l - 7 * m + 114) % 31) + 1 | |
| easter = dt.date(year, month_e, day_e) | |
| good_friday = easter - dt.timedelta(days=2) | |
| return good_friday | |
| except Exception as e: | |
| raise ValueError( | |
| f"Error calculating fallback Good Friday date for {year}: {str(e)}" | |
| ) from e | |
| except Exception as e: | |
| raise ValueError( | |
| f"Error determining Good Friday date for {year}: {str(e)} ({type(e)})" | |
| ) from e |
🧰 Tools
🪛 Ruff (0.8.2)
133-133: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
139-139: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
Summary by CodeRabbit
New Features
Improvements
Documentation
Chores
better-mdpackage as a new dependency.