Skip to content

Tslibs offsets validation #18210

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.

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

Closed
57 changes: 55 additions & 2 deletions pandas/_libs/tslibs/offsets.pyx
Original file line number Diff line number Diff line change
Expand Up @@ -212,13 +212,66 @@ def _validate_business_time(t_input):
raise ValueError("time data must match '%H:%M' format")
elif isinstance(t_input, dt_time):
if t_input.second != 0 or t_input.microsecond != 0:
raise ValueError(
"time data must be specified only with hour and minute")
raise ValueError("time data must be specified only with "
"hour and minute")
return t_input
else:
raise ValueError("time data must be string or datetime.time")


def _validate_n(n):
Copy link
Member

Choose a reason for hiding this comment

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

Docstring on all of these functions.

Copy link
Member Author

Choose a reason for hiding this comment

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

Will do.

"""
Require that `n` be a nonzero integer.

Parameters
----------
n : int

Raises
------
ValueError
"""
if n == 0 or not isinstance(n, int):
raise ValueError("N cannot be 0")


def _validate_month(month):
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 reason that these are not instance methods of BaseOffset?

Copy link
Member Author

Choose a reason for hiding this comment

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

My default is to not make something a method if it doesn't require self.

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 not the current style. agree that these are somewhat of an edge case, but don't change just for the sake of change or its 'your style', prefer to keep the existing code style unless good reason to change.

Copy link
Member Author

Choose a reason for hiding this comment

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

OK. The fix to the error message in validate_weekday is probably worthwhile. Should I revert the others?

Copy link
Contributor

Choose a reason for hiding this comment

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

I like them in cython, but these make more sense as methods I think as they do use self at least for some)

Copy link
Member Author

Choose a reason for hiding this comment

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

as they do use self at least for some

None of the ones implemented here.

Copy link
Contributor

Choose a reason for hiding this comment

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

right but the point is you don't need the allow_none arg if you simply make this a method (for the weekday one)

Copy link
Member Author

Choose a reason for hiding this comment

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

How does making it a method change whether allow_none is needed?

"""
Require that `month` be an integer between 1 and 12 inclusive.

Parameters
----------
month : int
Copy link
Contributor

Choose a reason for hiding this comment

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

I would guess we have this type of method in multiple places (though I agree these should actually be in this module).


Raises
------
ValueError
"""
if not isinstance(month, int) or not 1 <= month <= 12:
raise ValueError("Month must go from 1 to 12")


def _validate_weekday(weekday, allow_none=False):
"""
Copy link
Contributor

Choose a reason for hiding this comment

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

if this is a method, then you don't need allow_none (which is a really odd non-standard kw).

because you are a method, checking self.weekday is obvious.

now you can also have a _validate_weekday method that does everything (but the allow_none) and simply call that if you want.

If you show that you need these in multiple places, then certainly you can call them, from the offsets you need to have a method.

Copy link
Member Author

Choose a reason for hiding this comment

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

I still don't totally understand the reasoning, but it sounds like the upshot here is that to get this done _validate_weekday needs to become a method of _BaseOffset, and without a allow_none kwarg. Is that the correct takeaway?

Require that `weekday` be an integer between 0 and 6, inclusive, or that
None be explicitly allowed.

Parameters
----------
weekday : int (or None)
allow_none : bool, default False

Raises
------
ValueError
"""
if allow_none and weekday is None:
pass
elif not isinstance(weekday, int) or not 0 <= weekday <= 6:
raise ValueError("weekday must be 0<=weekday<=6, got "
"{day}".format(day=weekday))


# ---------------------------------------------------------------------
# Constructor Helpers

Expand Down
10 changes: 5 additions & 5 deletions pandas/tests/tseries/test_offsets.py
Original file line number Diff line number Diff line change
Expand Up @@ -2192,7 +2192,7 @@ def test_repr(self):
def test_corner(self):
pytest.raises(ValueError, Week, weekday=7)
tm.assert_raises_regex(
ValueError, "Day must be", Week, weekday=-1)
ValueError, "weekday must be", Week, weekday=-1)
Copy link
Member

Choose a reason for hiding this comment

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

Why are we changing all of these "Day" strings to "weekday" ?

Copy link
Member Author

Choose a reason for hiding this comment

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

Because the incorrect argument is "weekday". Many of these also take a "day" argument, so there is potential for ambiguity.


def test_isAnchored(self):
assert Week(weekday=0).isAnchored()
Expand Down Expand Up @@ -2263,9 +2263,9 @@ def test_constructor(self):
n=1, week=4, weekday=0)
tm.assert_raises_regex(ValueError, "^Week", WeekOfMonth,
n=1, week=-1, weekday=0)
tm.assert_raises_regex(ValueError, "^Day", WeekOfMonth,
tm.assert_raises_regex(ValueError, "^weekday", WeekOfMonth,
n=1, week=0, weekday=-1)
tm.assert_raises_regex(ValueError, "^Day", WeekOfMonth,
tm.assert_raises_regex(ValueError, "^weekday", WeekOfMonth,
n=1, week=0, weekday=7)

def test_repr(self):
Expand Down Expand Up @@ -2347,10 +2347,10 @@ def test_constructor(self):
tm.assert_raises_regex(ValueError, "^N cannot be 0",
LastWeekOfMonth, n=0, weekday=1)

tm.assert_raises_regex(ValueError, "^Day", LastWeekOfMonth, n=1,
tm.assert_raises_regex(ValueError, "^weekday", LastWeekOfMonth, n=1,
weekday=-1)
tm.assert_raises_regex(
ValueError, "^Day", LastWeekOfMonth, n=1, weekday=7)
ValueError, "^weekday", LastWeekOfMonth, n=1, weekday=7)

def test_offset(self):
# Saturday
Expand Down
33 changes: 10 additions & 23 deletions pandas/tseries/offsets.py
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,8 @@
from pandas._libs.tslibs.offsets import (
ApplyTypeError,
as_datetime, _is_normalized,
_get_firstbday, _get_calendar, _to_dt64, _validate_business_time,
_get_firstbday, _get_calendar, _to_dt64,
_validate_business_time, _validate_n, _validate_month, _validate_weekday,
_int_to_weekday, _weekday_to_int,
_determine_offset,
apply_index_wraps,
Expand Down Expand Up @@ -1398,10 +1399,7 @@ def __init__(self, n=1, normalize=False, weekday=None):
self.normalize = normalize
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))
_validate_weekday(weekday, allow_none=True)

self.kwds = {'weekday': weekday}

Expand Down Expand Up @@ -1490,12 +1488,8 @@ def __init__(self, n=1, normalize=False, week=None, weekday=None):
self.weekday = weekday
self.week = week

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

if self.weekday < 0 or self.weekday > 6:
raise ValueError('Day must be 0<=day<=6, got {day}'
.format(day=self.weekday))
_validate_n(n)
_validate_weekday(weekday)
if self.week < 0 or self.week > 3:
raise ValueError('Week must be 0<=week<=3, got {week}'
.format(week=self.week))
Expand Down Expand Up @@ -1586,12 +1580,8 @@ def __init__(self, n=1, normalize=False, weekday=None):
self.normalize = normalize
self.weekday = weekday

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

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

self.kwds = {'weekday': weekday}

Expand Down Expand Up @@ -1848,8 +1838,7 @@ 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')
_validate_month(month)

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

Expand Down Expand Up @@ -2102,8 +2091,7 @@ def __init__(self, n=1, normalize=False, weekday=0, startingMonth=1,
self.kwds = {'weekday': weekday, 'startingMonth': startingMonth,
'variation': variation}

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

if self.variation not in ["nearest", "last"]:
raise ValueError('{variation} is not a valid variation'
Expand Down Expand Up @@ -2354,8 +2342,7 @@ def __init__(self, n=1, normalize=False, weekday=0, startingMonth=1,
'qtr_with_extra_week': qtr_with_extra_week,
'variation': variation}

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

@cache_readonly
def _offset(self):
Expand Down