MNT: Cleanup FontProperties __init__ API #28843
Conversation
de3b37a to
d25ae3d
Compare
fc73e45 to
e1952bb
Compare
e1952bb to
b191200
Compare
b191200 to
782e613
Compare
lib/matplotlib/font_manager.py
Outdated
| 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])?
There was a problem hiding this comment.
👍 I thought we had something in cbook, but was looking for _is_nonstring_sequence 😆.
|
I guess this could do with some tests, mostly so I can see in which cases you do/don't intend to raise/warn? |
73cbd3d to
a6d8f41
Compare
|
Tests added. |
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.
a6d8f41 to
e8ac55a
Compare
| # 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).
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.
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.
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.
|
This would be a good use of the |
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_onlyor similar approaches fall short here.