Skip to content

MNT: Cleanup FontProperties __init__ API #28843

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.

Sign up for GitHub

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

Merged
merged 1 commit into from
Sep 26, 2024
Merged

MNT: Cleanup FontProperties __init__ API #28843

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 9 additions & 0 deletions doc/api/next_api_changes/deprecations/28843-TH.rst
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,9 @@
FontProperties initialization
~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

`.FontProperties` initialization is limited to the two call patterns:

- single positional parameter, interpreted as fontconfig pattern
- only keyword parameters for setting individual properties

All other previously supported call patterns are deprecated.
64 changes: 61 additions & 3 deletions lib/matplotlib/font_manager.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -32,6 +32,7 @@
import copy
import dataclasses
from functools import lru_cache
import functools
from io import BytesIO
import json
import logging
Expand Down Expand Up @@ -536,6 +537,57 @@ def afmFontProperty(fontpath, font):
return FontEntry(fontpath, name, style, variant, weight, stretch, size)


def _cleanup_fontproperties_init(init_method):
"""
A decorator to limit the call signature to single a positional argument
or alternatively only keyword arguments.

We still accept but deprecate all other call signatures.

When the deprecation expires we can switch the signature to::

__init__(self, pattern=None, /, *, family=None, style=None, ...)

plus a runtime check that pattern is not used alongside with the
keyword arguments. This results eventually in the two possible
call signatures::

FontProperties(pattern)
FontProperties(family=..., size=..., ...)

"""
@functools.wraps(init_method)
def wrapper(self, *args, **kwargs):
# multiple args with at least some positional ones
if len(args) > 1 or len(args) == 1 and kwargs:
# Note: Both cases were previously handled as individual properties.
# Therefore, we do not mention the case of font properties here.
_api.warn_deprecated(
"3.10",
message="Passing individual properties to FontProperties() "
"positionally was deprecated in Matplotlib %(since)s and "
"will be removed in %(removal)s. Please pass all properties "
"via keyword arguments."
)
# single non-string arg -> clearly a family not a pattern
if len(args) == 1 and not kwargs and not cbook.is_scalar_or_string(args[0]):
# Case font-family list passed as single argument
_api.warn_deprecated(
"3.10",
message="Passing family as positional argument to FontProperties() "
"was deprecated in Matplotlib %(since)s and will be removed "
"in %(removal)s. Please pass family names as keyword"
"argument."
)
# Note on single string arg:
# This has been interpreted as pattern so far. We are already raising if a
# non-pattern compatible family string was given. Therefore, we do not need
# to warn for this case.
return init_method(self, *args, **kwargs)

return wrapper


class FontProperties:
"""
A class for storing and manipulating font properties.
Expand Down Expand Up @@ -585,9 +637,14 @@ class FontProperties:
approach allows all text sizes to be made larger or smaller based
on the font manager's default font size.

This class will also accept a fontconfig_ pattern_, if it is the only
argument provided. This support does not depend on fontconfig; we are
merely borrowing its pattern syntax for use here.
This class accepts a single positional string as fontconfig_ pattern_,
or alternatively individual properties as keyword arguments::

FontProperties(pattern)
FontProperties(*, family=None, style=None, variant=None, ...)

This support does not depend on fontconfig; we are merely borrowing its
pattern syntax for use here.

.. _fontconfig: https://www.freedesktop.org/wiki/Software/fontconfig/
.. _pattern:
Expand All @@ -599,6 +656,7 @@ class FontProperties:
fontconfig.
"""

@_cleanup_fontproperties_init
def __init__(self, family=None, style=None, variant=None, weight=None,
stretch=None, size=None,
fname=None, # if set, it's a hardcoded filename to use
Expand Down
40 changes: 40 additions & 0 deletions lib/matplotlib/tests/test_font_manager.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,7 @@
import numpy as np
import pytest

import matplotlib as mpl
from matplotlib.font_manager import (
findfont, findSystemFonts, FontEntry, FontProperties, fontManager,
json_dump, json_load, get_font, is_opentype_cff_font,
Expand Down Expand Up @@ -367,3 +368,42 @@ def inner():
for obj in gc.get_objects():
if isinstance(obj, SomeObject):
pytest.fail("object from inner stack still alive")


def test_fontproperties_init_deprecation():
"""
Test the deprecated API of FontProperties.__init__.

The deprecation does not change behavior, it only adds a deprecation warning
via a decorator. Therefore, the purpose of this test is limited to check
which calls do and do not issue deprecation warnings. Behavior is still
tested via the existing regular tests.
"""
with pytest.warns(mpl.MatplotlibDeprecationWarning):
# multiple positional arguments
FontProperties("Times", "italic")

with pytest.warns(mpl.MatplotlibDeprecationWarning):
# Mixed positional and keyword arguments
FontProperties("Times", size=10)

with pytest.warns(mpl.MatplotlibDeprecationWarning):
# passing a family list positionally
FontProperties(["Times"])

# still accepted:
FontProperties(family="Times", style="italic")
FontProperties(family="Times")
FontProperties("Times") # works as pattern and family
FontProperties("serif-24:style=oblique:weight=bold") # pattern

# also still accepted:
# passing as pattern via family kwarg was not covered by the docs but
# historically worked. This is left unchanged for now.
# AFAICT, we cannot detect this: We can determine whether a string
# works as pattern, but that doesn't help, because there are strings
# that are both pattern and family. We would need to identify, whether
# 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")
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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).

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member Author

@timhoffm timhoffm Sep 23, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

2 changes: 1 addition & 1 deletion lib/matplotlib/ticker.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -574,7 +574,7 @@ def set_useMathText(self, val):
from matplotlib import font_manager
ufont = font_manager.findfont(
font_manager.FontProperties(
mpl.rcParams["font.family"]
family=mpl.rcParams["font.family"]
),
fallback_to_default=False,
)
Expand Down
Loading