Validate text rotation in setter #19228
Conversation
lib/matplotlib/text.py
Outdated
| if isinstance(s, str): | ||
| _api.check_in_list(['vertical', 'horizontal'], rotation=s) | ||
| elif s is not None: | ||
| cbook._check_isinstance(numbers.Real, s=s) |
There was a problem hiding this comment.
either both should be s=s, or both should be rotation=s, depending on whether you want to strictly match the kwarg name, or use a semantically more meaningful name.
There was a problem hiding this comment.
Both should be rotation=s. It's understandable in the context of set_rotation() as well as in Text(..., rotation=).
For the latter s=s would result in "'invalid string' is not a valid value for s" which is quite unhelpful.
| @@ -232,7 +232,7 @@ def test_add_subplot_invalid(): | |||
| def test_suptitle(): | |||
| fig, _ = plt.subplots() | |||
| fig.suptitle('hello', color='r') | |||
| fig.suptitle('title', color='g', rotation='30') | |||
| fig.suptitle('title', color='g', rotation=30) | |||
There was a problem hiding this comment.
I guess strictly speaking this is backcompat breaking? I doubt it's worth a deprecation period, but it should be noted.
There was a problem hiding this comment.
Not even sure we need to mention it. '30' is not a supported format according to docs. It just happens to work. If we drop that without deprecation period, a note does not really help anybody. Nobody goes through the note and says "hey rotation does not take string numbers anymore. That's my thing, I'm doing that all the time". I'll bet anything if anybody passes '30', they will only notice by the error message they will get.
dstansby
force-pushed
the
validate-rotation
branch
from
37ee7e6 to
4a92d39
Compare
|
@dstansby please change to |
dstansby
force-pushed
the
validate-rotation
branch
from
4a92d39 to
b59c831
Compare
dstansby
force-pushed
the
validate-rotation
branch
2 times, most recently
from
d021b94 to
d845a57
Compare
|
I finally got round to rebasing this, so should be good for review again. |
|
Uses |
lib/matplotlib/text.py
Outdated
| @@ -1177,6 +1178,10 @@ def set_rotation(self, s): | |||
| The rotation angle in degrees in mathematically positive direction | |||
| (counterclockwise). 'horizontal' equals 0, 'vertical' equals 90. | |||
| """ | |||
| if isinstance(s, str): | |||
There was a problem hiding this comment.
The two separate checks lead to slightly confusing messages as they don't mention the respective other valid parameters, e.g.
ValueError: '30' is not a valid value for rotation; supported values are 'vertical', 'horizontal'
does not mention numbers.
I'd go for a manual check and message here:
if s is not None and not isinstance(s, numbers.Real) and s not in ['vertical', 'horizontal']:
Raise ValueError("rotation must be 'vertical', 'horizontal' or a number, not {s!r}"]
dstansby
force-pushed
the
validate-rotation
branch
from
d845a57 to
02ec590
Compare
xref #19223. I think checking for
numbers.Realis the right thing to do here?