Skip to content

FIX: skip sub directories when finding fonts on windows #22909

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 1 commit into from Apr 28, 2022
Merged

FIX: skip sub directories when finding fonts on windows #22909

merged 1 commit into from Apr 28, 2022

Conversation

ghost
Copy link

@ghost ghost commented Apr 26, 2022 •
edited by ghost
Loading

add win32 fonts directory check

PR Summary

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

github-actions[bot]
github-actions bot reviewed Apr 26, 2022
View reviewed changes
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 Apr 26, 2022
@tacaswell
Copy link
Member

Thank you for working on this @Croadden

The test failures are real and caused by these changes because win32FontDirectory uses some windows-only modules. This logic will have to be behind some platfrom gating. I suspect that 👍🏻

if sys.platfrom == 'win32' and ...:

would be enough because and will short circuit and not evaluate the second expression if the first is False.

Could you please also give the PR title and the commit messages more descriptive text (like "FIX: skip sub directories when finding fonts on windows") so that someone who is coming across this issue / commit in the future will have are very quick suggestion as to the motivation for the change. What files you changed and the actually changes is in the diff so the commit message is a chance for you to tell the future readers why you made this change!

@ghost ghost changed the title #22859 bug fix FIX: skip sub directories when finding fonts on windows, bug #22859 Apr 26, 2022
@ghost
Copy link
Author

ghost commented Apr 26, 2022

Damn, was pretty sure I saw OS check :(
My bad, will fix. Sorry, I'm kinda new at github and whole this open source stuff

@ghost
Copy link
Author

ghost commented Apr 26, 2022

@tacaswell, I was pretty sure I used propper commit naming... Also I'm not able to rename commits without force pushing and rebasing, so maybe lets leave it as is for now? Lesson learnt.

@ghost
Copy link
Author

ghost commented Apr 26, 2022

Oh, found it! I'm using github desktop and I eddited commit description, but not commit summary. So that's why commit messages is Update font_manager.py instead of add win32 fonts directory check and FIX: skip sub directories when finding fonts on windows

@ghost ghost changed the title FIX: skip sub directories when finding fonts on windows, bug #22859 FIX: skip sub directories when finding fonts on windows Apr 27, 2022
tacaswell
tacaswell reviewed Apr 27, 2022
View reviewed changes
lib/matplotlib/font_manager.py Outdated
for filename in filenames
if Path(filename).suffix.lower() in extensions]
if sys.platform == 'win32' and directory == win32FontDirectory():
return [os.path.join(directory, filename) for filename in os.listdir(directory) if os.path.isfile(filename)]
Copy link
Member

Choose a reason for hiding this comment

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

I am very confused why this is not failing the linter as too long of a line.....

Copy link
Member

Choose a reason for hiding this comment

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

Because we explicitly exclude font_manager.py from the line length check

matplotlib/.flake8

Line 60 in 1ab4a53

lib/matplotlib/font_manager.py: E501

Copy link
Author

Choose a reason for hiding this comment

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

I am very confused why this is not failing the linter as too long of a line.....

I'm running flake8 before each commit I make, so I think it's just ignored

Copy link
Author

Choose a reason for hiding this comment

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

Oh, wrote previous comment before you replied to me :)
So should I fix it or not?

@tacaswell
Copy link
Member

Oh, found it!

I'm glad you sorted it out! The summary (e.g. the first line) is very helpful when skimming the history.

Unfortunately there is no way to update the commit message (which even though the UI shows it as two things is really just 1 block of text) is one of the values that git hashes to compute the SHA of the commit (which while annoying here, it means that someone can not change history without someone noticing!). As changing the message changes the SHA (and hence history) you have to force-push to publish the new commits. Again, git is trying to be helpful here and make sure that you do not accidentally delete history (think of the case where two people are working on the same branch. If both fetch the upstream branch, do some work and commit. The first person to push it works, the second person to push it fails (because if it did work it would effectively delete the commit from the first person). From git's point of view it can not tell "I change a commit and want to update it" from "two people are trying to push completely different work" so push says no, but git push --force-with-lease says "trust me git, I know what I am doing!".

Sorry for the wall of text, hopefully it is more helpful than harmful.

@ghost
Copy link
Author

ghost commented Apr 27, 2022

Your wall of the text help me so much, thanks! :)

But I made a lot of mistakes here. Sorry for being bad, my git experience is so low (was using SVN only for more than 5 years)
I fixes the issue with naming using rebase and everything seems fine in git and git gui (I have only 2 commits with proper naming here), but history seems broken here, at github. So what should I do? Maybe create another branch, delete this one and make another pull request?

@tacaswell
Copy link
Member

@Croadden may I fix it and force-push to your branch?

@tacaswell tacaswell linked an issue Apr 28, 2022 that may be closed by this pull request
[Bug]: findSystemFonts should not look in subdirectories of C:\Windows\Fonts\ #22859
Closed
@ghost
Copy link
Author

ghost commented Apr 28, 2022

@tacaswell Sure, no problem, it's just simple reformat

@timhoffm
Copy link
Member

@tacaswell as you go, you can include my suggestion above.

@Vladislav-Bartalevich @timhoffm @tacaswell
skip sub directories when finding fonts on windows
e800616 
Closes #22859

Co-authored-by: Tim Hoffmann <2836374+timhoffm@users.noreply.github.com>
@tacaswell
Copy link
Member

tacaswell commented Apr 28, 2022 •
edited
Loading

Done!

Steps I took:

  • use the GH ui to commit @timhoffm 's suggestion
  • checked out the branch locally with the name fix_win32_fonts
  • git rebase -i back to where this forked from master (I used magit in emacs to set this up)
  • squashed everything down into one commit via the UI (could have maybe dropped duplicate commits, but squashing and fixing conflicts seemed like it would be faster)
  • fixed up a conflict as the rebase went
  • force pushed to your main branch: git push -v --force-with-lease Croadden fix_win32_fonts:refs/heads/main (again actually done through magit)
  • wrote this note

@ghost
Copy link
Author

ghost commented Apr 28, 2022

Thanks!

@QuLogic QuLogic merged commit 4756fa2 into matplotlib:main Apr 28, 2022
@QuLogic
Copy link
Member

QuLogic commented Apr 28, 2022

Thanks @Croadden! Congratulations on your first PR to Matplotlib 🎉 We hope to hear from you again.

@ghost
Copy link
Author

ghost commented Apr 28, 2022

Will do my best :)
Thanks!

@anntzer anntzer mentioned this pull request Dec 7, 2022
Merged
@almarklein almarklein mentioned this pull request Dec 7, 2022
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
v3.6.0
Development

Successfully merging this pull request may close these issues.

[Bug]: findSystemFonts should not look in subdirectories of C:\Windows\Fonts\
4 participants
@tacaswell @timhoffm @QuLogic @Vladislav-Bartalevich