Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Lock down kwargs in offsets signatures #17458

Merged
merged 22 commits into from
Oct 6, 2017
Merged

Lock down kwargs in offsets signatures #17458

Show file tree
Hide file tree
Changes from 13 commits
Commits
Show all changes
22 commits
Select commit Hold shift + click to select a range
5e276c9
Lock down kwargs in offsets signatures
jbrockmendel Sep 7, 2017
0183e50
whatsnew note
jbrockmendel Sep 7, 2017
e33626e
Merge branch 'master' of https://github.com/pandas-dev/pandas into of…
jbrockmendel Sep 8, 2017
1377df1
Merge branch 'master' of https://github.com/pandas-dev/pandas into of…
jbrockmendel Sep 24, 2017
ed78a4a
restrict YearOffset to month kwarg
jbrockmendel Sep 24, 2017
36290d2
Restrict SemiMonthOffset kwargs to day_of_month
jbrockmendel Sep 24, 2017
4443d6c
Lock down kwargs in FY5253 and FY5253Quarter
jbrockmendel Sep 24, 2017
8cb6b66
lock down kwargs in BusinessDay, CustomBusinessMonthEnd
jbrockmendel Sep 24, 2017
fe7bb56
Lockdown kwargs in remaining DateOffset subclasses
jbrockmendel Sep 24, 2017
3daab66
Briefer WhatsNew, add missing kwds attrs
jbrockmendel Sep 24, 2017
aefb68c
whitespace fixups
jbrockmendel Sep 24, 2017
1e06d99
Move doc section to other api changes bullet point
jbrockmendel Sep 26, 2017
7be31da
Merge branch 'master' into offset_sigs
jbrockmendel Sep 26, 2017
cd1f224
Add pickle tests for 0.19.2 and 0.20.3
jbrockmendel Sep 29, 2017
97c896e
Merge branch 'offset_sigs' of https://github.com/jbrockmendel/pandas …
jbrockmendel Sep 29, 2017
e93de50
Merge branch 'master' of https://github.com/pandas-dev/pandas into of…
jbrockmendel Sep 29, 2017
7426265
Merge branch 'master' of https://github.com/pandas-dev/pandas into of…
jbrockmendel Oct 2, 2017
aee75de
New version of test_pickle_v0_20_3
jbrockmendel Oct 2, 2017
cc5a71a
flake8 whitespace fixup
jbrockmendel Oct 3, 2017
521a8d0
Merge branch 'master' of https://github.com/pandas-dev/pandas into of…
jbrockmendel Oct 3, 2017
217a558
remove test per reviewer instruction
jbrockmendel Oct 4, 2017
086b485
Add offsets with kwds to generate_legacy_storage_files
jbrockmendel Oct 6, 2017
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions doc/source/whatsnew/v0.21.0.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -482,6 +482,7 @@ Other API Changes
- :func:`to_datetime` when passed a tz-aware ``origin=`` kwarg will now raise a more informative ``ValueError`` rather than a ``TypeError`` (:issue:`16842`)
- Renamed non-functional ``index`` to ``index_col`` in :func:`read_stata` to improve API consistency (:issue:`16342`)
- Bug in :func:`DataFrame.drop` caused boolean labels ``False`` and ``True`` to be treated as labels 0 and 1 respectively when dropping indices from a numeric index. This will now raise a ValueError (:issue:`16877`)
- Restricted DateOffset keyword arguments. Previously, ``DateOffset`` subclasses allowed arbitrary keyword arguments which could lead to unexpected behavior. Now, only valid arguments will be accepted. (:issue:`17176`).

.. _whatsnew_0210.deprecations:

Expand Down
141 changes: 78 additions & 63 deletions pandas/tseries/offsets.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -581,6 +581,7 @@ def __setstate__(self, state):
if '_offset' in state: # pragma: no cover
raise ValueError('Unexpected key `_offset`')
state['_offset'] = state.pop('offset')
state['kwds']['offset'] = state['_offset']
self.__dict__ = state
if 'weekmask' in state and 'holidays' in state:
calendar, holidays = _get_calendar(weekmask=self.weekmask,
Expand All @@ -598,11 +599,11 @@ class BusinessDay(BusinessMixin, SingleConstructorOffset):
_prefix = 'B'
_adjust_dst = True

def __init__(self, n=1, normalize=False, **kwds):
def __init__(self, n=1, normalize=False, offset=timedelta(0)):
self.n = int(n)
self.normalize = normalize
self.kwds = kwds
self._offset = kwds.get('offset', timedelta(0))
self.kwds = {'offset': offset}
self._offset = offset

def _offset_str(self):
def get_str(td):
Expand Down Expand Up @@ -693,14 +694,13 @@ def onOffset(self, dt):

class BusinessHourMixin(BusinessMixin):

def __init__(self, **kwds):
def __init__(self, start='09:00', end='17:00', offset=timedelta(0)):
# must be validated here to equality check
kwds['start'] = self._validate_time(kwds.get('start', '09:00'))
kwds['end'] = self._validate_time(kwds.get('end', '17:00'))
kwds = {'offset': offset}
self.start = kwds['start'] = self._validate_time(start)
self.end = kwds['end'] = self._validate_time(end)
self.kwds = kwds
self._offset = kwds.get('offset', timedelta(0))
self.start = kwds.get('start', '09:00')
self.end = kwds.get('end', '17:00')
self._offset = offset

def _validate_time(self, t_input):
from datetime import time as dt_time
Expand Down Expand Up @@ -923,10 +923,11 @@ class BusinessHour(BusinessHourMixin, SingleConstructorOffset):
_prefix = 'BH'
_anchor = 0

def __init__(self, n=1, normalize=False, **kwds):
def __init__(self, n=1, normalize=False, start='09:00',
end='17:00', offset=timedelta(0)):
self.n = int(n)
self.normalize = normalize
super(BusinessHour, self).__init__(**kwds)
super(BusinessHour, self).__init__(start=start, end=end, offset=offset)

@cache_readonly
def next_bday(self):
Expand Down Expand Up @@ -960,11 +961,11 @@ class CustomBusinessDay(BusinessDay):
_prefix = 'C'

def __init__(self, n=1, normalize=False, weekmask='Mon Tue Wed Thu Fri',
holidays=None, calendar=None, **kwds):
holidays=None, calendar=None, offset=timedelta(0)):
self.n = int(n)
self.normalize = normalize
self.kwds = kwds
self._offset = kwds.get('offset', timedelta(0))
self._offset = offset
self.kwds = {}

calendar, holidays = _get_calendar(weekmask=weekmask,
holidays=holidays,
Expand All @@ -976,6 +977,7 @@ def __init__(self, n=1, normalize=False, weekmask='Mon Tue Wed Thu Fri',
self.kwds['weekmask'] = self.weekmask = weekmask
self.kwds['holidays'] = self.holidays = holidays
self.kwds['calendar'] = self.calendar = calendar
self.kwds['offset'] = offset

@apply_wraps
def apply(self, other):
Expand Down Expand Up @@ -1026,10 +1028,12 @@ class CustomBusinessHour(BusinessHourMixin, SingleConstructorOffset):
_anchor = 0

def __init__(self, n=1, normalize=False, weekmask='Mon Tue Wed Thu Fri',
holidays=None, calendar=None, **kwds):
holidays=None, calendar=None,
start='09:00', end='17:00', offset=timedelta(0)):
self.n = int(n)
self.normalize = normalize
super(CustomBusinessHour, self).__init__(**kwds)
super(CustomBusinessHour, self).__init__(start=start,
end=end, offset=offset)

calendar, holidays = _get_calendar(weekmask=weekmask,
holidays=holidays,
Expand Down Expand Up @@ -1121,7 +1125,7 @@ class SemiMonthOffset(DateOffset):
_default_day_of_month = 15
_min_day_of_month = 2

def __init__(self, n=1, day_of_month=None, normalize=False, **kwds):
def __init__(self, n=1, normalize=False, day_of_month=None):
Copy link
Contributor

Choose a reason for hiding this comment

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

is this consistent with other freq signatures?

Copy link
Member Author

Choose a reason for hiding this comment

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

All the other signatures are (self, n=1, normalize=False, .... In the status quo this is an outlier.

Copy link
Contributor

Choose a reason for hiding this comment

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

yes that's fine. see my comment below though.

Copy link
Contributor

Choose a reason for hiding this comment

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

this is fine, in an ideal world we would change it but can't now

if day_of_month is None:
self.day_of_month = self._default_day_of_month
else:
Expand All @@ -1132,8 +1136,7 @@ def __init__(self, n=1, day_of_month=None, normalize=False, **kwds):
day=self.day_of_month))
self.n = int(n)
self.normalize = normalize
self.kwds = kwds
self.kwds['day_of_month'] = self.day_of_month
self.kwds = {'day_of_month': self.day_of_month}

@classmethod
def _from_name(cls, suffix=None):
Expand Down Expand Up @@ -1408,18 +1411,19 @@ class CustomBusinessMonthEnd(BusinessMixin, MonthOffset):
_prefix = 'CBM'

def __init__(self, n=1, normalize=False, weekmask='Mon Tue Wed Thu Fri',
holidays=None, calendar=None, **kwds):
holidays=None, calendar=None, offset=timedelta(0)):
self.n = int(n)
self.normalize = normalize
self.kwds = kwds
self._offset = kwds.get('offset', timedelta(0))
self._offset = offset
self.kwds = {}

calendar, holidays = _get_calendar(weekmask=weekmask,
holidays=holidays,
calendar=calendar)
self.kwds['weekmask'] = self.weekmask = weekmask
self.kwds['holidays'] = self.holidays = holidays
self.kwds['calendar'] = self.calendar = calendar
self.kwds['offset'] = offset

@cache_readonly
def cbday(self):
Expand All @@ -1430,7 +1434,7 @@ def cbday(self):
def m_offset(self):
kwds = self.kwds
kwds = {key: kwds[key] for key in kwds
if key not in ['calendar', 'weekmask', 'holidays']}
if key not in ['calendar', 'weekmask', 'holidays', 'offset']}
return MonthEnd(n=1, normalize=self.normalize, **kwds)

@apply_wraps
Expand Down Expand Up @@ -1478,20 +1482,21 @@ class CustomBusinessMonthBegin(BusinessMixin, MonthOffset):
_prefix = 'CBMS'

def __init__(self, n=1, normalize=False, weekmask='Mon Tue Wed Thu Fri',
holidays=None, calendar=None, **kwds):
holidays=None, calendar=None, offset=timedelta(0)):
self.n = int(n)
self.normalize = normalize
self.kwds = kwds
self._offset = kwds.get('offset', timedelta(0))
self._offset = offset
self.kwds = {}

# _get_calendar does validation and possible transformation
# of calendar and holidays.
calendar, holidays = _get_calendar(weekmask=weekmask,
holidays=holidays,
calendar=calendar)
kwds['calendar'] = self.calendar = calendar
kwds['weekmask'] = self.weekmask = weekmask
kwds['holidays'] = self.holidays = holidays
self.kwds['calendar'] = self.calendar = calendar
self.kwds['weekmask'] = self.weekmask = weekmask
self.kwds['holidays'] = self.holidays = holidays
self.kwds['offset'] = offset

@cache_readonly
def cbday(self):
Expand All @@ -1502,7 +1507,7 @@ def cbday(self):
def m_offset(self):
kwds = self.kwds
kwds = {key: kwds[key] for key in kwds
if key not in ['calendar', 'weekmask', 'holidays']}
if key not in ['calendar', 'weekmask', 'holidays', 'offset']}
return MonthBegin(n=1, normalize=self.normalize, **kwds)

@apply_wraps
Expand Down Expand Up @@ -1540,17 +1545,17 @@ class Week(DateOffset):
_adjust_dst = True
_inc = timedelta(weeks=1)

def __init__(self, n=1, normalize=False, **kwds):
def __init__(self, n=1, normalize=False, weekday=None):
self.n = n
self.normalize = normalize
self.weekday = kwds.get('weekday', None)
self.weekday = weekday

if self.weekday is not None:
if self.weekday < 0 or self.weekday > 6:
raise ValueError('Day must be 0<=day<=6, got {day}'
.format(day=self.weekday))

self.kwds = kwds
self.kwds = {'weekday': weekday}

def isAnchored(self):
return (self.n == 1 and self.weekday is not None)
Expand Down Expand Up @@ -1642,9 +1647,9 @@ class WeekOfMonth(DateOffset):
Parameters
----------
n : int
week : {0, 1, 2, 3, ...}
week : {0, 1, 2, 3, ...}, default None
0 is 1st week of month, 1 2nd week, etc.
weekday : {0, 1, ..., 6}
weekday : {0, 1, ..., 6}, default None
0: Mondays
1: Tuesdays
2: Wednesdays
Expand All @@ -1656,11 +1661,11 @@ class WeekOfMonth(DateOffset):

_adjust_dst = True

def __init__(self, n=1, normalize=False, **kwds):
def __init__(self, n=1, normalize=False, week=None, weekday=None):
self.n = n
self.normalize = normalize
self.weekday = kwds['weekday']
self.week = kwds['week']
self.weekday = weekday
self.week = week

if self.n == 0:
raise ValueError('N cannot be 0')
Expand All @@ -1672,7 +1677,7 @@ def __init__(self, n=1, normalize=False, **kwds):
raise ValueError('Week must be 0<=week<=3, got {week}'
.format(week=self.week))

self.kwds = kwds
self.kwds = {'weekday': weekday, 'week': week}

@apply_wraps
def apply(self, other):
Expand Down Expand Up @@ -1742,21 +1747,22 @@ class LastWeekOfMonth(DateOffset):

Parameters
----------
n : int
weekday : {0, 1, ..., 6}
n : int, default 1
weekday : {0, 1, ..., 6}, default None
0: Mondays
1: Tuesdays
2: Wednesdays
3: Thursdays
4: Fridays
5: Saturdays
6: Sundays

"""

def __init__(self, n=1, normalize=False, **kwds):
def __init__(self, n=1, normalize=False, weekday=None):
Copy link
Contributor

Choose a reason for hiding this comment

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

is there a test for this as weekday=None? isn't that an error?

Copy link
Member Author

Choose a reason for hiding this comment

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

It is an error, though a different one in Py2 vs Py3. In the status quo not passing a weekday explicitly will raise a KeyError. Default to Monday I guess?

Copy link
Contributor

Choose a reason for hiding this comment

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

no if weekday is None, just raise a value error, in reality this should actually be the first (and a required parameter), but I guess that would be hard to change. Don't set defaults, raise

Copy link
Member Author

Choose a reason for hiding this comment

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

Are you suggesting it should be __init__(self, weekday, n=1, normalize=False)? I see some appeal to that, but there is a lot of symmetry with other signatures all starting with (self, n=1, normalize=False, ...).

Copy link
Contributor

Choose a reason for hiding this comment

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

see my comment above. this is ok for now. actually weekday is optional, so this is fine.

Copy link
Member Author

Choose a reason for hiding this comment

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

Two things. First, here and in other places you say "this is fine" but I don't know whether you're referring to the PR or the thing the PR is fixing. Based on the go-ahead you've given below to look over timeseries.rst I'm assuming you mean the PR version is fine, just heads up for ways I'm liable to get confused next time around.

actually weekday is optional,

Under the status quo, failing to pass weekday raises a KeyError. In the PR, not passing anything will raise a ValueError in py2 and TypeError in py3. (The same errors would result in the status quo if a user passed None explicitly)

Copy link
Contributor

Choose a reason for hiding this comment

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

I never mean this entire PR is fine. If that was the case I wouldn't have any other comments. Individual comments are revering to the specific line/section where I make it.

self.n = n
self.normalize = normalize
self.weekday = kwds['weekday']
self.weekday = weekday

if self.n == 0:
raise ValueError('N cannot be 0')
Expand All @@ -1765,7 +1771,7 @@ def __init__(self, n=1, normalize=False, **kwds):
raise ValueError('Day must be 0<=day<=6, got {day}'
.format(day=self.weekday))

self.kwds = kwds
self.kwds = {'weekday': weekday}

@apply_wraps
def apply(self, other):
Expand Down Expand Up @@ -1829,13 +1835,14 @@ class QuarterOffset(DateOffset):
# TODO: Consider combining QuarterOffset and YearOffset __init__ at some
# point

def __init__(self, n=1, normalize=False, **kwds):
def __init__(self, n=1, normalize=False, startingMonth=None):
Copy link
Contributor

Choose a reason for hiding this comment

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

can you make an issue that this should be deprecated to starting_month (and prob others like this)

self.n = n
self.normalize = normalize
self.startingMonth = kwds.get('startingMonth',
self._default_startingMonth)
if startingMonth is None:
startingMonth = self._default_startingMonth
self.startingMonth = startingMonth

self.kwds = kwds
self.kwds = {'startingMonth': startingMonth}

def isAnchored(self):
return (self.n == 1 and self.startingMonth is not None)
Expand Down Expand Up @@ -2017,13 +2024,14 @@ class YearOffset(DateOffset):
"""DateOffset that just needs a month"""
_adjust_dst = True

def __init__(self, n=1, normalize=False, **kwds):
self.month = kwds.get('month', self._default_month)
def __init__(self, n=1, normalize=False, month=None):
month = month if month is not None else self._default_month
self.month = month

if self.month < 1 or self.month > 12:
raise ValueError('Month must go from 1 to 12')

DateOffset.__init__(self, n=n, normalize=normalize, **kwds)
DateOffset.__init__(self, n=n, normalize=normalize, month=month)

@classmethod
def _from_name(cls, suffix=None):
Expand Down Expand Up @@ -2262,15 +2270,17 @@ class FY5253(DateOffset):
_suffix_prefix_nearest = 'N'
_adjust_dst = True

def __init__(self, n=1, normalize=False, **kwds):
def __init__(self, n=1, normalize=False, weekday=0, startingMonth=1,
variation="nearest"):
self.n = n
self.normalize = normalize
self.startingMonth = kwds['startingMonth']
self.weekday = kwds["weekday"]
self.startingMonth = startingMonth
self.weekday = weekday

self.variation = kwds["variation"]
self.variation = variation

self.kwds = kwds
self.kwds = {'weekday': weekday, 'startingMonth': startingMonth,
'variation': variation}

if self.n == 0:
raise ValueError('N cannot be 0')
Expand Down Expand Up @@ -2510,24 +2520,29 @@ class FY5253Quarter(DateOffset):
_prefix = 'REQ'
_adjust_dst = True

def __init__(self, n=1, normalize=False, **kwds):
def __init__(self, n=1, normalize=False, weekday=0, startingMonth=1,
qtr_with_extra_week=1, variation="nearest"):
self.n = n
self.normalize = normalize

self.qtr_with_extra_week = kwds["qtr_with_extra_week"]
self.weekday = weekday
self.startingMonth = startingMonth
self.qtr_with_extra_week = qtr_with_extra_week
self.variation = variation

self.kwds = kwds
self.kwds = {'weekday': weekday, 'startingMonth': startingMonth,
'qtr_with_extra_week': qtr_with_extra_week,
'variation': variation}

if self.n == 0:
raise ValueError('N cannot be 0')

@cache_readonly
def _offset(self):
kwds = self.kwds
return FY5253(
startingMonth=kwds['startingMonth'],
weekday=kwds["weekday"],
variation=kwds["variation"])
startingMonth=self.startingMonth,
weekday=self.weekday,
variation=self.variation)

def isAnchored(self):
return self.n == 1 and self._offset.isAnchored()
Expand Down