Add parameter checks to DayLocator initiator #6955
Conversation
| @@ -1254,6 +1241,12 @@ def __init__(self, bymonthday=None, interval=1, tz=None): | |||
|
|
|||
| Default is to tick every day of the month: ``bymonthday=range(1,32)`` | |||
| """ | |||
| if interval < 1: | |||
| raise ValueError("The interval parameter must be an integer " | |||
There was a problem hiding this comment.
since these are distinct errors, the messages should probabably be unique:
interval < 1 : The interval must be greater than 0
isinstance(interval, int): the interval must be an integer
(and also, this should then maybe be a TypeError?)
Otherwise you may as well do if interval <1 or not isinstance(interval, int)
There was a problem hiding this comment.
That makes sense. I thought of both cases. On the one hand, I feel since the parameter is being addressed anyways, it is worthwhile to flat out say both limitations. But on the other, if interval <1 or not isinstance(interval, int) started looking a little full. I figured it was best to just open the PR and get feedback.
I think it is still the better option. I will consolidate it to one check/one exception.
|
I'm confused as to what happened to the one Travis build. Someone mind taking a look? I reran the mathtext test locally and it was no problem. |
| @@ -1254,6 +1241,8 @@ def __init__(self, bymonthday=None, interval=1, tz=None): | |||
|
|
|||
| Default is to tick every day of the month: ``bymonthday=range(1,32)`` | |||
| """ | |||
| if not isinstance(interval, int) or interval < 1: | |||
There was a problem hiding this comment.
This will fail on internal == 1.0 which is surprising. I think if not interval == int(interval) is a better test here.
There was a problem hiding this comment.
That is a fair point, but for a day locator, does that really make sense? I personally think it would be better to enforce the typing in this specific case.
Unless you think it would be a problem with existing code bases, then I suppose catering to compatibility would be preferred.
There was a problem hiding this comment.
It has a chance of breaking user code. This also breaks in the case of non-python ints, ex
In [56]: d = np.uint8(5)
In [57]: isinstance(d, int)
Out[59]: False
In [60]: d == int(d)
Out[60]: Trueisinstance should only be used when there is no other reasonable option.
There was a problem hiding this comment.
Aha yes that makes sense, especially with the numpy example. I remember now having some nightmares with Qt qints in an old app. I will get that fixed up in the morning.
|
For reasons un-known the math text tests are flaky, restarted the tests. |
Check that interval parameter is an integer greater than zero. Delete unuseful 'optimization' meant to prevent exceeding the MAXTICKS variable. During testing it seemed ineffective. The following Locator.raise_if_exceeds exception was triggered first anyways. resolves matplotlib#6935
LindyBalboa
force-pushed
the
issue_6935
branch
from
85a5c4a
to
b5c9aa4
Compare
|
Looks good. Can you add a test for that exception being raised on invalid input? |
MNT: Add parameter checks to DayLocator initiator
|
backported to V2.x as 125a296 |
QuLogic
modified the milestones:
2.0 (style change major release),
2.0.1 (next bug fix release)
|
modify lib/matplotlib/ticker.py |
Check that interval parameter is an integer greater than zero.
Delete unuseful 'optimization' meant to prevent exceeding the
MAXTICKS variable. During testing it seemed ineffective. The
following Locator.raise_if_exceeds exception was triggered first
anyways.
resolves #6935