Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork7.9k
Log a warning if selected font weight differs from requested#30272
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
Conversation
lib/matplotlib/font_manager.pyi Outdated
| @@ -19,6 +18,7 @@ MSUserFontDirectories: list[str] | |||
| X11FontDirectories:list[str] | |||
| OSXFontDirectories:list[str] | |||
| def_normalize_weight(weight:str|Number)->Number: ... | |||
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.
Typical font weights are multiples of 100. Can / should this be any Number or can we specialize to int?
| def_normalize_weight(weight:str|Number)->Number: ... | |
| def_normalize_weight(weight:str|int)->int: ... |
If we want a general Number, there should be a test case for it.
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.
Looking at the OpenType spec, it does appear to be integral only:https://learn.microsoft.com/en-us/typography/opentype/spec/os2#usweightclass
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.
Also, I think the original PR just usedNumber because it was already imported infont_manager.py.
| def test_map_weights(): | ||
| assert _normalize_weight('ultralight') == 100 |
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.
Let's reframe this to test the helper instead of the mapping, so that we can also test the passthrough behavior
| deftest_map_weights(): | |
| assert_normalize_weight('ultralight')==100 | |
| deftest_normalize_weight(): | |
| assert_normalize_weight(300)==300# passthough | |
| assert_normalize_weight('ultralight')==100 |
Maybe also add an error case e.g. "italic"
lib/matplotlib/font_manager.py Outdated
| if best_font is not None: | ||
| if (_normalize_weight(prop.get_weight()) != | ||
| _normalize_weight(best_font.weight)): |
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.
Why are these nested ifs instead ofif A and B ?
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.
One minor suggestion to improve the changelog - take it or leave it, and feel free to self merge.
| ``font_manager.findfont`` logs if selected font weight does not match requested | ||
| ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ |
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.
| ``font_manager.findfont``logs if selected font weight does not match requested | |
| ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ | |
| ``font_manager.findfont``warns if selected the font weight does not match requested | |
| ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ |
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.
No, it's not a warning.
| def test_font_match_warning(caplog): | ||
| findfont(FontProperties(family=["DejaVu Sans"], weight=750)) | ||
| logs = [rec.message for rec in caplog.records] | ||
| assert 'findfont: Failed to find font weight 750, now using 700.' in logs |
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.
Just for my own interest, is there a reason to check the logs, instead of using apytest.warns context?
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.
Same reason as above.
d2050cb intomatplotlib:mainUh oh!
There was an error while loading.Please reload this page.
PR summary
This is a rebase of the orphaned#15615, but with@anntzer's comments handled.
PR checklist