-
-
Notifications
You must be signed in to change notification settings - Fork 7.6k
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
FIX: make set_text(None) keep string empty instead of "None" #10392
FIX: make set_text(None) keep string empty instead of "None" #10392
Conversation
There's a bunch of places where passing in None is interpreted as the string "None" (e.g. xlabel, title) and I would rather have them all fixed at once rather than in a piecemeal fashion. |
Yeah, OK, fair enough. I'd propse in def set_text(self, s):
"""
Set the text string *s*
It may contain newlines (``\\n``) or math in LaTeX syntax.
ACCEPTS: string or anything printable with '%s' conversion.
"""
if s is not None:
self._text = '%s' % (s,)
self.stale = True |
probably an API change note |
lib/matplotlib/text.py
Outdated
""" | ||
self._text = '%s' % (s,) | ||
self.stale = True | ||
if s is not None: |
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.
shouldn't you always clear the text (and mark as stale) when s is None?
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.
Oh, good question! Open to suggestions
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.
Set to empty string if None
is passed.
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.
There's a lot to say here:
- IMO
'%s' % (s,)
is an anti-pattern. It is identical tostr(s)
. self._text
is not defined in__init__
. It only callsset_text(text)
. Something likeText(text=None).get_text()
would fail with an attibute error in your code. It's good practice to declare your attributes in init. You should addself._text = ''
to init.- I would expect that
set_text(None)
andset_text('')
are completely equivalent. - Minor thing: Add a colon at the end of the sentence in the docstring.
So either always stale:
def set_text(s):
self._text = str(s) if s is not None else ''
self.stale = True
or stale only when there's really a change:
def set_text(s):
if s is None:
s = ''
if s != self._text:
self._text = s
self.stale = True
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.
Great catch. You're right I didn't notice that self._text
wasn't being set.
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.
@timhoffm not sure where you want the colon...
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.
Sorry, full stop: "Set the text string s."
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.
Great. Fixed. Also, your logic was cleaner, so used that (second option).
fig, ax = plt.subplots() | ||
ax.plot(range(10)) | ||
leg = ax.legend() | ||
assert leg.get_title().get_text() == "" |
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.
assert not leg.get_title().get_text()
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 don't think we should rely on boolean-ness here.
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.
This is PEP-8 convention
For sequences, (strings, lists, tuples), use the fact that empty sequences are false.
But I'm ok, if you want to have an explicit comparison.
lib/matplotlib/text.py
Outdated
|
||
It may contain newlines (``\\n``) or math in LaTeX syntax. | ||
|
||
ACCEPTS: string or anything printable with '%s' conversion. | ||
ACCEPTS: string or anything printable with '%s' conversion, except |
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.
"anything printable with '%s' conversion" is technically correct, but feels a bit beside the point. This has nothing to do with printing. I don't really know a good alternative.
According to __str__
docs "anything with an "informal" string representation" would be right, but I'm afraid that users will not understand this.
Maybe "anything that can be formatted by '%s'" or "anything that can be converted by '%s'"?
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.
Actually any object has a __str__
... (yes, you can technically make __str__
raise an exception but whatever).
The test should more be something like "any object, will be cast to a string using str
" (don't care much for Py2 now especially if this goes to mpl3.0...).
lib/matplotlib/text.py
Outdated
|
||
It may contain newlines (``\\n``) or math in LaTeX syntax. | ||
|
||
ACCEPTS: string or anything printable with '%s' conversion. | ||
ACCEPTS: string or anything printable with '%s' conversion, except | ||
``None``, which leaves the text string empty: |
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.
which is equivalent to an empty string.
@timhoffm python 2.7 gave unicode errors for |
53e8eec
to
bb9a330
Compare
@jklymak Ok, leaving the |
@tacaswell so for 3.0 milestones PRs can we ignore failing 2.7 tests? If so, then I can implement @timhoffm's |
Approved, whatever the decision on |
d824411
to
7bbf4e5
Compare
Thanks for your help @timhoffm |
OK, have gone back to the cast if this is milestones 3.0. Should pass the 3.x tests. Modified the docstring as per @anntzer comment above. |
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.
Looks great. I'll merge pending successful CI
Just to be clear, 2.7 tests/doc build will fail. But if this is 3.0 those tests should be ignored, right @tacaswell? |
Circle CI failure seems unrelated:
u'\u2212' is the "minus sign" |
it’s definitley because set_text calls str(s) now. If I take that out the docs build. |
@jklymak technically |
Well, I'll just leave it the way it was before I touched anything.... We can revisit for python 3... |
This seemed to be ready to go, so I used "squash and merge". Let's see if that broke anything. |
Could someone also check to make sure that ax.get_legend().get_visible() is working properly? |
What does working properly mean in this context? How do you expect it to behave and why? |
After executing ax.get_legend().set_visible(False) I would like ax.get_legend().get_visible() to return False. Another issue that I think needs to be fixed is that after setting ax.get_legend().set_visible(False) and then back True again, all the specifics of my legend are lost (legend position, marker size, etc) |
Can you provide a MSCE that has this problem? I can't reproduce. If you do provide an example, please open a new issue... If I do: import matplotlib.pyplot as plt
fig, ax = plt.subplots()
ax.plot(range(10), label='Boo')
leg = ax.legend()
print(leg)
ax.get_legend().set_visible(False)
print(ax.get_legend().get_visible())
ax.get_legend().set_visible(True)
print(ax.get_legend().get_visible())
plt.show() I get
and the legend appears as expected... If I don't |
I'm not using plt since my plot is embedded in Tkinter. My code is pretty complicated, it's python 2.7 on a RaspberryPi with matplotlib 2.1. I was hoping someone else would have run into the same issue and found an easy fix or workaround. If not, I will just live with it. The other issue of losing the legend parameters between making visible and invisible seems like a more serious flaw but I don't really want to go through the hassle of simplifying my code to provide a MSCE and doing whatever is needed to open a new issue. I'm kind of a newb on here. This is just my good deed of bringing it to someone's attention. |
The example above doesn't have that behaviour, so its pretty hard to chase down what the issue might be that you are having. If you figure it out, let us know. |
PR Summary
As documented in #10391,
legend.set_title()
doesn't do sensible things with the defaulttitle=None
(i.e it set the string of the legend title to the string "None").This is kind of an API change in that if anyone was checking for the string
"None"
to see if the legend title was empty will now fail, but lets hope no one was doing that.As pointed out below by @anntzer the problem is more wide spread: The updated PR now checks if
s
isNone
inText.set_text
and if it is does not do anything. Before it would set the string to"None"
which is silly.One could go further and insist that
s
is a string (so the user knows what they are doing), but that'll likely break lots of people assuming the string cast happens.This still needs a test....PR Checklist