Skip to content
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Prev Previous commit
Next Next commit
don't accept, only raise with more informative exception
  • Loading branch information
dontgoto committed Mar 26, 2024
commit c24b1431fe4ee89cd6cdcd23e291f56c5e4ec2d3
1 change: 0 additions & 1 deletion doc/source/whatsnew/v3.0.0.rst
Original file line number Diff line number Diff line change
Expand Up @@ -280,7 +280,6 @@ Performance improvements

Bug fixes
~~~~~~~~~
- Fixed bug in :class:`Series.Holiday` that leads to a ``TypeError`` when using ``Holiday.dates`` with a ``Holiday`` that is initialized with ``offset`` of type ``list`` which itself references another Holiday's list of offsets. (:issue:`29049`)
- Fixed bug in :class:`SparseDtype` for equal comparison with na fill value. (:issue:`54770`)
- Fixed bug in :meth:`DataFrame.join` inconsistently setting result index name (:issue:`55815`)
- Fixed bug in :meth:`DataFrame.to_string` that raised ``StopIteration`` with nested DataFrames. (:issue:`16098`)
Expand Down
46 changes: 18 additions & 28 deletions pandas/tests/tseries/holiday/test_holiday.py
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,6 @@
from pandas import (
DatetimeIndex,
Series,
to_datetime,
)
import pandas._testing as tm

Expand Down Expand Up @@ -199,33 +198,6 @@ def test_holidays_within_dates(holiday, start, expected):
) == [utc.localize(dt) for dt in expected]


def test_holidays_within_dates_offset_of_offset():
# see gh-29049
# Test that the offset of an offset is correctly applied to the holiday
# And that dates can be calculated
holiday1 = Holiday(
"Holiday1",
month=USThanksgivingDay.month,
day=USThanksgivingDay.day,
offset=[USThanksgivingDay.offset, DateOffset(1)],
)
holiday2 = Holiday(
"Holiday2",
month=holiday1.month,
day=holiday1.day,
offset=[holiday1.offset, DateOffset(3)],
)
# there shall be no lists of lists here
for offset in holiday2.offset:
assert isinstance(offset, DateOffset)

min_date, max_date = (to_datetime(x) for x in ["2017-11-1", "2018-11-30"])
expected_min, expected_max = DatetimeIndex(["2017-11-27", "2018-11-26"])
actual_min, actual_max = holiday2.dates(min_date, max_date)
assert actual_min == expected_min
assert actual_max == expected_max


@pytest.mark.parametrize(
"transform", [lambda x: x.strftime("%Y-%m-%d"), lambda x: Timestamp(x)]
)
Expand Down Expand Up @@ -299,6 +271,24 @@ def test_both_offset_observance_raises():
)


def test_list_of_list_of_offsets_raises():
# see gh-29049
# Test that the offsets of offsets are forbidden
holiday1 = Holiday(
"Holiday1",
month=USThanksgivingDay.month,
day=USThanksgivingDay.day,
offset=[USThanksgivingDay.offset, DateOffset(1)],
)
with pytest.raises(ValueError, match="Nested lists are not supported for offset"):
Holiday(
"Holiday2",
month=holiday1.month,
day=holiday1.day,
offset=[holiday1.offset, DateOffset(3)],
)


def test_half_open_interval_with_observance():
# Prompted by GH 49075
# Check for holidays that have a half-open date interval where
Expand Down
19 changes: 7 additions & 12 deletions pandas/tseries/holiday.py
Original file line number Diff line number Diff line change
Expand Up @@ -166,7 +166,7 @@ def __init__(
year=None,
month=None,
day=None,
offset: BaseOffset | list[BaseOffset | list[BaseOffset]] | None = None,
offset: BaseOffset | list[BaseOffset] | None = None,
observance: Callable | None = None,
start_date=None,
end_date=None,
Expand Down Expand Up @@ -235,22 +235,17 @@ class from pandas.tseries.offsets, default None
"""
if offset is not None and observance is not None:
raise NotImplementedError("Cannot use both offset and observance.")
if isinstance(offset, list) and any(isinstance(off, list) for off in offset):
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
if offset is not None and observance is not None:
raise NotImplementedError("Cannot use both offset and observance.")
if isinstance(offset, list) and any(isinstance(off, list) for off in offset):
if offset is not None:
if observance is not None:
raise NotImplementedError("Cannot use both offset and observance.")
if not (isinstance(offset, DateOffset) or (isinstance(list, DateOffset) and all(isinstance(off, DateOffset) for off in offsets))):
raise ValueError(
"Nested lists are not supported for offset. "
"Flatten composite offsets of `Holiday.offset`s first."
Copy link
Member

Choose a reason for hiding this comment

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

This should probably just say what is allowed for offset instead of a specific failure case

)

self.name = name
self.year = year
self.month = month
self.day = day
self.offset: None | BaseOffset | list[BaseOffset]
if isinstance(offset, list):
self.offset = []
for off in offset:
# check if we are handling composite offsets of another holiday
if isinstance(off, list):
self.offset.extend(off)
else:
self.offset.append(off)
else:
self.offset = offset
self.offset = offset
self.start_date = (
Timestamp(start_date) if start_date is not None else start_date
)
Expand Down