Skip to content

Fix Path/str-discrepancy in FontManager.addpath and improve documentation #22591

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

Jump to bottom
Merged
merged 4 commits into from
Mar 21, 2022
Merged

Fix Path/str-discrepancy in FontManager.addpath and improve documentation #22591

merged 4 commits into from
Mar 21, 2022

Conversation

oscargus
Copy link
Member

@oscargus oscargus commented Mar 3, 2022 •
edited
Loading

PR Summary

Closes #22582
Closes #22586

Always convert input to str first.

Also update the documentation a bit.

Cannot really see how this can be tested as I do not know of any ttf-font locations on the CI... Edit: I found a ttf-font in the test data, so at least it should be possible to test passing a Path without errors. Edit 2: Test added.

Also added a validation function for font.stretch, so now rcParam-values can also be ints.

Kept it as four separate commits, but can of course merge to a single.

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

@oscargus oscargus force-pushed the fontmanageraddpathstr branch from f56ea70 to e22b3a6 Compare March 3, 2022 18:14
@oscargus oscargus added this to the v3.6.0 milestone Mar 3, 2022
@oscargus oscargus force-pushed the fontmanageraddpathstr branch 2 times, most recently from 9939784 to 22e3daf Compare March 3, 2022 18:25
QuLogic
QuLogic reviewed Mar 3, 2022
View reviewed changes
@oscargus oscargus marked this pull request as draft March 4, 2022 08:05
@oscargus oscargus force-pushed the fontmanageraddpathstr branch 2 times, most recently from 6d5edf6 to 63eba3b Compare March 4, 2022 19:19
@oscargus oscargus changed the title Fix Path/str-discrepancy in FontManager.addpath Fix Path/str-discrepancy in FontManager.addpath and improve documentation Mar 4, 2022
@oscargus oscargus marked this pull request as ready for review March 4, 2022 19:40
timhoffm
timhoffm reviewed Mar 6, 2022
View reviewed changes
Copy link
Member

@timhoffm timhoffm left a comment

Choose a reason for hiding this comment

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

This is good, only some stylistic comments.

lib/matplotlib/font_manager.py

- style: Either 'normal' (default), 'italic' or 'oblique'.
- style: Either 'normal', 'italic' or 'oblique'.
Copy link
Member

Choose a reason for hiding this comment

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

Optional: Highlight the properties using italics (or alternatively literal format)

Suggested change
- style: Either 'normal', 'italic' or 'oblique'.
- *style*: Either 'normal', 'italic' or 'oblique'.

This is also quite redundant with the setters. This is almost completely a data class. IMHO it would be reasonable to add properties and move all the documentation there, i.e. delete this list and the setter docstrings (setters may link to the property docs. But that's for a later time.

Copy link
Member Author

Choose a reason for hiding this comment

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

That would make sense, but I think it is better to try to get the information from the setmethods at a later stage.

@oscargus oscargus marked this pull request as draft March 6, 2022 10:26
@oscargus oscargus force-pushed the fontmanageraddpathstr branch from 63eba3b to 9e67574 Compare March 14, 2022 09:25
@oscargus oscargus marked this pull request as ready for review March 14, 2022 09:27
@oscargus
Copy link
Member Author

oscargus commented Mar 14, 2022 •
edited
Loading

I changed based on the comments.

However, there is still the same issue as in #22556 (comment)

Edit: I screwed up when editing and did just amend to the last commit rather than updating the correct one, so one may want to squash.

@oscargus oscargus force-pushed the fontmanageraddpathstr branch from 9e67574 to 249f360 Compare March 14, 2022 11:10
@oscargus oscargus force-pushed the fontmanageraddpathstr branch from 249f360 to 0525305 Compare March 15, 2022 08:22
oscargus
oscargus commented Mar 15, 2022
View reviewed changes
lib/matplotlib/font_manager.py
@@ -1,7 +1,7 @@
"""
A module for finding, managing, and using fonts across platforms.

This module provides a single `FontManager` instance that can
This module provides a single `FontManager` instance, ``fontManager``, that can
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 did not manage to link to fontManager when the definition is in font_manager_api.rst.

@oscargus oscargus force-pushed the fontmanageraddpathstr branch from 0525305 to 7f21bca Compare March 15, 2022 11:51
@QuLogic QuLogic merged commit e3c8195 into matplotlib:main Mar 21, 2022
@twm
Copy link

twm commented Mar 22, 2022

Thank you all very much for the quick resolution!

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

@anntzer anntzer anntzer left review comments

@QuLogic QuLogic QuLogic approved these changes

@timhoffm timhoffm timhoffm approved these changes

Assignees
No one assigned
Labels
Documentation topic: text/fonts
Projects
None yet
Milestone
v3.6.0
5 participants
@oscargus @twm @QuLogic @anntzer @timhoffm