Skip to content

Lock down kwargs in offsets signatures #17458

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

Merged
merged 22 commits into from
Oct 6, 2017
Merged

Lock down kwargs in offsets signatures #17458

Changes from 1 commit
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
45 changes: 20 additions & 25 deletions pandas/tseries/offsets.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1571,18 +1571,18 @@ class Week(DateOffset):
"""
_adjust_dst = True

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._inc = timedelta(weeks=1)
self.kwds = kwds
self.kwds = {'weekday': weekday}

def isAnchored(self):
return (self.n == 1 and self.weekday is not None)
Expand Down Expand Up @@ -1674,9 +1674,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 @@ -1688,11 +1688,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 @@ -1704,7 +1704,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 @@ -1774,21 +1774,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 @@ -1797,7 +1798,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 @@ -1861,13 +1862,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 @@ -1985,13 +1987,6 @@ class QuarterEnd(QuarterOffset):
_default_startingMonth = 3
_prefix = 'Q'

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

self.kwds = kwds

def isAnchored(self):
return (self.n == 1 and self.startingMonth is not None)

Expand Down