-
-
Notifications
You must be signed in to change notification settings - Fork 8k
Validate text rotation in setter #19228
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
Conversation
lib/matplotlib/text.py
Outdated
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.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
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.
modulo my comment, which is not strictly necessary, though.
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.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
37ee7e6 to
4a92d39
Compare
@dstansby please change to cbook._check_isinstance(numbers.Real, rotation=s). See the open comment above.
4a92d39 to
b59c831
Compare
f6e0ee4 to
a1eef38
Compare
d021b94 to
d845a57
Compare
I finally got round to rebasing this, so should be good for review again.
Uses rotation='30'. Please change to int.
lib/matplotlib/text.py
Outdated
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.
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}"]
d845a57 to
02ec590
Compare
xref #19223. I think checking for
numbers.Realis the right thing to do here?