-
-
Notifications
You must be signed in to change notification settings - Fork 18.4k
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
Tslibs offsets validation #18210
Changes from 6 commits
b3d4325
ada3854
9ef49b4
94bb286
f9ebf6a
2f6ce82
44d0c4d
ba12543
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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): | ||
""" | ||
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): | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. is there a reason that these are not instance methods of BaseOffset? There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe 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? There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
None of the ones implemented here. There was a problem hiding this comment. Choose a reason for hiding this commentThe 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) There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. How does making it a method change whether |
||
""" | ||
Require that `month` be an integer between 1 and 12 inclusive. | ||
|
||
Parameters | ||
---------- | ||
month : int | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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): | ||
""" | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 |
||
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 | ||
|
||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Why are we changing all of these "Day" strings to "weekday" ? There was a problem hiding this comment. Choose a reason for hiding this commentThe 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() | ||
|
@@ -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): | ||
|
@@ -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 | ||
|
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Will do.