Deprecate non-string values as legend labels #16771
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.
Can you add a note suggesting that they probably wanted to pass in two lists?
There was a problem hiding this comment.
I'd rather not. This check is now implemented in Legend with signature Legend(parent, handles, labels, ..). So the "two lists" does not apply anymore. Otherwise I'd have to make the check in mlegend._parse_legend_args(). But then the parameter in Legend is not checked explicitly - was not before, but I think it should anyway. Furthermore, you can pass labels positionally or by kwarg to legend(); 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 call legend(lines) with the message Passing 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.
I suspect that we are still going to get some confused users, but this is certainly clearer than it currently is!
9b7d171 to
1e1804e
Compare
|
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? |
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.