Movatterモバイル変換

[0]ホーム
URL:

Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

 Sign up
Appearance settings

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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Merged
timhoffm merged 2 commits intomatplotlib:mainfromojeda-e:issue21761
Dec 7, 2021

Conversation

ojeda-e
Copy link
Contributor

@ojeda-eojeda-e commentedNov 29, 2021•
edited
Loading

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 (andpytest passes).
  • IsFlake 8 compliant (installflake8-docstrings and runflake8 --docstring-convention=all).

Documentation

  • New features are documented, with examples if plot related.
  • New features have an entry indoc/users/next_whats_new/ (follow instructions in README.rst there).
  • [N/A] API changes documented indoc/api/next_api_changes/ (follow instructions in README.rst there).
  • Documentation is sphinx and numpydoc compliant (the docs shouldbuild 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 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 pagetext_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 infont_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-actionsgithub-actionsbot 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 uson gitter for real-time discussion.

For details on testing, writing docs, and our review process, please seethe developer guide

We strive to be a welcoming and open project. Please follow ourCode of Conduct.

@tacaswelltacaswell added this to thev3.6.0 milestoneNov 30, 2021
QuLogic added a commit to QuLogic/matplotlib that referenced this pull requestDec 1, 2021
@ojeda-e
Copy link
ContributorAuthor

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
ContributorAuthor

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

@ojeda-eojeda-e changed the titleAdded get_font_list() to fontManagerAdded get_font_names() to fontManagerDec 4, 2021
@ojeda-e
Copy link
ContributorAuthor

Thanks for reviewing@timhoffm. Following your suggestion, I changedget_font_list toget_font_names.

@ojeda-e
Copy link
ContributorAuthor

Thanks,@timhoffm for the hint! The main reason to add the change in regex in this PR is to make thepr_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 previouspr_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

For simplicity, let's just leave as is.

ojeda-e reacted with thumbs up emoji

@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

@tacaswelltacaswell 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
ContributorAuthor

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.

ojeda-e reacted with thumbs up emoji

@timhoffmtimhoffm merged commit801d1b7 intomatplotlib:mainDec 7, 2021
@timhoffm
Copy link
Member

Thanks@ojeda-e!

ojeda-e reacted with hooray emoji

Sign up for freeto join this conversation on GitHub. Already have an account?Sign in to comment
Reviewers

@github-actionsgithub-actions[bot]github-actions[bot] left review comments

@tacaswelltacaswelltacaswell approved these changes

@timhoffmtimhoffmtimhoffm approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
v3.6.0
Development

Successfully merging this pull request may close these issues.

[Doc]: add how to know available fonts...
3 participants
@ojeda-e@timhoffm@tacaswell
[8]ページ先頭
©2009-2025 Movatter.jp