-
-
Notifications
You must be signed in to change notification settings - Fork 7.9k
Added get_font_names() to fontManager #21799
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
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for opening your first PR into Matplotlib!
If you have not heard from us in a while, please feel free to ping @matplotlib/developers
or anyone who has commented on the PR. Most of our reviewers are volunteers and sometimes things fall through the cracks.
You can also join us on gitter for real-time discussion.
For details on testing, writing docs, and our review process, please see the developer guide
We strive to be a welcoming and open project. Please follow our Code of Conduct.
As noted in gitter for matplotlib#21799.
Thanks for the suggestion @tacaswell, I changed the regex in |
Thanks for reviewing @timhoffm. Following your suggestion, I changed |
Thanks, @timhoffm for the hint! The main reason to add the change in regex in this PR is to make the |
For simplicity, let's just leave as is. |
This is adding a new function to our public API so it it is a new feature! I think this is something that people have been asking for, not always in ways that either they or we understood them to be asking for this, for a while now. I think that seeing "You can now get the names of the fonts Matplotlib knows about!" will make lightbulbs go off for some people. We also have ~monthly bug reports where someone is reading a newer version of the docs than they have Matplotlib installed so it is convenient in those cases to be able to link them to the whats-new entry for the feature. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this should get a whats-new entry, but not going to block merging over it.
Thanks for your feedback @tacaswell. I'll add an entry to whats-new and next-API-changes then! :D |
Just whats new, the api changes is for document when and why we intentionally changed behavior that we expect to break existing code. |
Thanks @ojeda-e! |
PR Summary
Added list of available fonts. Changes in this PR allows the user to find a list of available fonts by running:
matplotlib.font_manager.get_font_list()
. Edit: Fixes #21761.PR Checklist
Tests and Styling
pytest
passes).flake8-docstrings
and runflake8 --docstring-convention=all
).Documentation
doc/users/next_whats_new/
(follow instructions in README.rst there).doc/api/next_api_changes/
(follow instructions in README.rst there).Comments
About get_font_list()
I added get_font_list() as a method in FontManager. However, for convenience, I also added it as global function. In this way, although the method itself belongs to FontManager (
matplotlib.font_manager.fontManager.get_font_list()
) , is easily accessible one level up atmatplotlib.font_manager.get_font_list()
.Test Units
Although I was considering the use of a fixture to extract all the fonts from scratch but ended up skipping it. In the spirit of best practices, the addition of a fixture should be for usability across different unit tests, which in this case does not occur. The unit test is a bit long for my preference, but I tried my best to overcome the limitations.
Documentation
I added a note in the tutorial page
text_props.py
where there is a reference about fonts. I am not sure about adding plots here to illustrate the different font options. I could add a section if the reviewers consider it appropriate.New features
Didn't add new features as I am not entirely sure this change is substantial enough to be added to the list of new features. I can add it later otherwise.
Finally, as a general comment, I would like to point out that the code in
font_manager.py
could be further improved as well as the unit tests associated with it. I guess because the last changes in this part of the library were added quite some time ago. I also understand the limitations when testing fonts across different platforms, though. Anyway, If there is interest in the enhancement of this component of matplotlib I can offer some help, but that's another discussion.Let's start here, will be happy to improve/change whatever is necessary. Thanks.