Skip to content
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 our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Move towards making texmanager stateless. #22725

Merged
merged 1 commit into from Mar 31, 2022
Merged

Conversation

anntzer
Copy link
Contributor

@anntzer anntzer commented Mar 29, 2022

Previously, TexManager needed to call get_font_config at a specific
place in the middle of processing to update some internal attributes
before proceeding with TeX source generation. Instead, move towards
making TexManager stateless (except for caching), i.e. the user facing
API should be thought of as a bunch of independently callable functions
make_tex(), make_dvi(), etc. (they will probably stay as methods on
a "empty" TexManager object for a long time for backcompat, in fact).

PR Summary

PR Checklist

Tests and Styling

  • Has pytest style unit tests (and pytest passes).
  • Is Flake 8 compliant (install flake8-docstrings and run flake8 --docstring-convention=all).

Documentation

  • New features are documented, with examples if plot related.
  • New features have an entry in doc/users/next_whats_new/ (follow instructions in README.rst there).
  • API changes documented in doc/api/next_api_changes/ (follow instructions in README.rst there).
  • Documentation is sphinx and numpydoc compliant (the docs should build without error).

@timhoffm
Copy link
Member

I suggest to make all stateless functions classmethods to make it very clear that they do not depend on state.

@anntzer
Copy link
Contributor Author

anntzer commented Mar 29, 2022

Good idea, done.

lib/matplotlib/texmanager.py Show resolved Hide resolved
lib/matplotlib/texmanager.py Outdated Show resolved Hide resolved
for font_family in cls._font_families:
if is_reduced_font and font_family == requested_family:
preambles[font_family] = cls._font_preambles[
rcParams['font.family'][0].lower()]
Copy link
Member

Choose a reason for hiding this comment

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

Um, what is this? I thought rcParams['font.family'] is a string?!? Should this be

Suggested change
rcParams['font.family'][0].lower()]
rcParams['font.family'].lower()]

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No, it's a list of strings:

"font.family": validate_stringlist, # used by text object

Copy link
Member

@timhoffm timhoffm Mar 29, 2022

Choose a reason for hiding this comment

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

Ah, I was checking

## The font.family property can take either a concrete font name (not supported

and
#font.family: sans-serif

We should make that clearer in matplotlibrc.

And yes interestingly validate_stringlist can make a list out of a single string. matplotlibrc is less well-defined than I thought 😦.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This could reasonably be tracked as a separate issue?

Copy link
Member

Choose a reason for hiding this comment

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

Sure: #22746.

Previously, TexManager needed to call get_font_config at a specific
place in the middle of processing to update some internal attributes
before proceeding with TeX source generation.  Instead, move towards
making TexManager stateless (except for caching), i.e. the user facing
API should be thought of as a bunch of independently callable functions
`make_tex()`, `make_dvi()`, etc. (they will probably stay as methods on
a "empty" TexManager object for a long time for backcompat, in fact).
@timhoffm timhoffm added this to the v3.6.0 milestone Mar 31, 2022
@timhoffm timhoffm merged commit a5fec21 into matplotlib:main Mar 31, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants