Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork7.9k
Deprecate non-string values as legend labels#16771
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Uh oh!
There was an error while loading.Please reload this page.
Conversation
| if not isinstance(label, str): | ||
| cbook.warn_deprecated( | ||
| "3.3", | ||
| message="Passing non-string objects as legend " |
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.
Can you add a note suggesting that they probably wanted to pass in two lists?
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'd rather not. This check is now implemented inLegend with signatureLegend(parent, handles, labels, ..). So the "two lists" does not apply anymore. Otherwise I'd have to make the check inmlegend._parse_legend_args(). But then the parameter inLegend is not checked explicitly - was not before, but I think it should anyway. Furthermore, you can passlabels positionally or by kwarg tolegend(); while the str-check would apply to both, the proposed addition would only make sense for the former.
On the other hand, IMHO it's clear enough to respond on a calllegend(lines) with the messagePassing non-string objects as legend labels is deprecated. It's rather obvious from this that we interpret the only parameter as labels.
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 suspect that we are still going to get some confused users, but this is certainly clearer than it currently is!
There are a lot of places where we accept any object and cast them to strings (e.g. titles, xlabels, etc.) Do we want to move all of them to only accept strings for consistency (I would probably support that)? Alternatively, if we want to get rid of the (very real) confusion of users passing artists to legend(), do we just want to deprecate passing Artists to legend() (which is almost certainly a bug), rather than deprecate all non-strs? |
timhoffm commentedMar 21, 2020 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
Gone with that solution (see#16864). We can defer the decision if we want to have a strict str-checking all over the library. |
Closing in favor of#16864. |
PR Summary
Closes#16567.