MNT: Cleanup FontProperties __init__ API - #28843
ksunden merged 1 commit into
Conversation
Sorry, something went wrong.
| if (len(args) == 1 and not kwargs | ||
| and isinstance(args[0], Iterable) and not isinstance(args[0], str)): |
There was a problem hiding this comment.
Is this not cbook.is_scalar_or_string(args[0])?
Sorry, something went wrong.
There was a problem hiding this comment.
👍 I thought we had something in cbook, but was looking for _is_nonstring_sequence 😆.
Sorry, something went wrong.
|
I guess this could do with some tests, mostly so I can see in which cases you do/don't intend to raise/warn? |
Sorry, something went wrong.
|
Tests added. |
Sorry, something went wrong.
The cleanup approach is to not modify code logic during a deprecation period, but only detect deprecated call patterns through a decorator and warn on them.
| # a string is *not* a valid family. | ||
| # Since this case is not covered by docs, I've refrained from jumping | ||
| # extra hoops to detect this possible API misuse. | ||
| FontProperties(family="serif-24:style=oblique:weight=bold") |
There was a problem hiding this comment.
The docstring on FontProperties says that family should be a list. However, the docstring on set_family does say it can be a list or a string and we just fall back to that internally.
If the family is passed as a str we can detect that and ask the user to either pass it positionally (if they meant it to be a pattern) or as a list (if they meant it to be a family).
Sorry, something went wrong.
There was a problem hiding this comment.
I think we need to deal with this if we want to push this as eventually we can end up with patterns coming in to family via the kwarg which will stop working with no warning.
Sorry, something went wrong.
There was a problem hiding this comment.
I would rather keep accepting a string as family, because it's consistent with set_family and it's convenient. For now, I'd still accept a pattern as family kwarg as undocumented backward-compatibility. We can decide separately whether we want to move away from that.
Sorry, something went wrong.
There was a problem hiding this comment.
TL;DR This PR only deprecates working but undocumented and unintended behavior. After deprecation, we will have to logically separted APIs (positional pattern or kwargs). We do change the officially documented API. - That could be done in a later step if desired (and will become simpler though this PR).
This would be a good use of the FontProperties.from_pattern(...) but it is probably not worth the effort to push.
That was my initial approach at #28837. I think it's eventually still a good idea. In the current apporach, I'm going less far in that I narrow the API to the documented behavior. Users, who have used FontPropertie as documented will not need to change anything. This approach already seperates the API logically (mostly if you still support the special case of family=pattern). The nice thing here is that we do the deprecation solely by a decorator, i.e. don't run into the danger of accidentally breaking an currently valid code through singature changes. We can later deprecate FontProperties(pattern) in favor of FontProperties.from_pattern, but that can be a separate discussion.
Sorry, something went wrong.
|
This would be a good use of the FontProperties.from_pattern(...) but it is probably not worth the effort to push. |
Sorry, something went wrong.
The cleanup approach is to not modify code logic during a deprecation period, but only detect deprecated call patterns through a decorator and warn on them.
Alternative to #28837.
Essentially, the cleanup limits the API to the intended call patterns. This will subsequently make it easier and more explicit to distinguish between the two intended call patterns "one positional argument as pattern" or "individual properties as keyword arguments" and resolves weird corner cases and unexpected behavior of the current API.
The cleanup approach is to not modify code logic during a deprecation period, but only detect deprecated call patterns through a custom decorator and warn on them. This is needed due to the somewhat particular dual signature. Standard _make_keyword_only or similar approaches fall short here.