Skip to content

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

Merged
merged 2 commits into from
Dec 7, 2021
Merged

Conversation

ojeda-e
Copy link
Contributor

@ojeda-e ojeda-e commented Nov 29, 2021

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

  • 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).
  • [N/A] 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).

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 at matplotlib.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.

Copy link

@github-actions github-actions bot left a 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.

@tacaswell tacaswell added this to the v3.6.0 milestone Nov 30, 2021
QuLogic added a commit to QuLogic/matplotlib that referenced this pull request Dec 1, 2021
@ojeda-e
Copy link
Contributor Author

ojeda-e commented Dec 2, 2021

Thanks for the suggestion @tacaswell, I changed the regex in .github/workflows/clean_pr.yml. The added unit test passes although it skips Windows platforms for now. Let me know if the PR needs further changes/improvements. I'll be happy to answer questions if any. Thanks :)

@ojeda-e
Copy link
Contributor Author

ojeda-e commented Dec 2, 2021

Now that tests are passing, can I have a quick review here by @jklymak or @dstansby?
Thanks.

@ojeda-e ojeda-e changed the title Added get_font_list() to fontManager Added get_font_names() to fontManager Dec 4, 2021
@ojeda-e
Copy link
Contributor Author

ojeda-e commented Dec 4, 2021

Thanks for reviewing @timhoffm. Following your suggestion, I changed get_font_list to get_font_names.

@ojeda-e
Copy link
Contributor Author

ojeda-e commented Dec 5, 2021

Thanks, @timhoffm for the hint! The main reason to add the change in regex in this PR is to make the pr_clean action pass, but you're right, and I shouldn't mix these two.
PR #21833 aims to fix the regex syntax. What do you suggest in this scenario? If I restore the previous pr_clean action, it will fail in this PR. Should I wait for PR #21833 to close first? Then I could rebase and the two issues will be kept separate.

@timhoffm
Copy link
Member

timhoffm commented Dec 5, 2021

For simplicity, let's just leave as is.

@tacaswell
Copy link
Member

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.

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.

Copy link
Member

@tacaswell tacaswell left a 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.

@ojeda-e
Copy link
Contributor Author

ojeda-e commented Dec 6, 2021

Thanks for your feedback @tacaswell. I'll add an entry to whats-new and next-API-changes then! :D

@tacaswell
Copy link
Member

Just whats new, the api changes is for document when and why we intentionally changed behavior that we expect to break existing code.

@timhoffm timhoffm merged commit 801d1b7 into matplotlib:main Dec 7, 2021
@timhoffm
Copy link
Member

timhoffm commented Dec 7, 2021

Thanks @ojeda-e!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Doc]: add how to know available fonts...
3 participants