Skip to content
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

Various small fixes #53

Merged
merged 9 commits into from
Jan 29, 2023
Merged

Various small fixes #53

merged 9 commits into from
Jan 29, 2023

Conversation

seproDev
Copy link
Contributor

@seproDev seproDev commented Jan 28, 2023

Describe your changes
Various small changes and fixes, the commit messages should be pretty descriptive.

Related issue
Closes #50, Closes #51, Closes #52
I added the option to pass through all kwargs instead of just lazy. This partially addresses #49 through the fontNumber option. I think the issue should probably stay open though, since I feel like a nicer interface to interact with collections is still needed.

Checklist before requesting a review

  • I have performed a self-review of my code.
  • I have added thorough tests for the proposed changes.
  • I have run the tests and there are no errors. (test_get_fingerprint_diff_between_variable_instances and test_str fail, but they also fail on main for me. Only test_str fails in main for me.)

Without deleting the object the file stays locked preventing the removal of the temporary folder
@seproDev
Copy link
Contributor Author

seproDev commented Jan 28, 2023

Okay, seems like 1494afd causes the test failure. Though I am not entirely sure why. I am pretty certain the commit is correct. Should I just drop it for now?

Edit: I adjusted the get_fingerprint() function. I believe all tests should pass now. Though if you would rather I drop both commits, I can do that to.

@fabiocaccamo fabiocaccamo self-assigned this Jan 29, 2023
@codecov
Copy link

codecov bot commented Jan 29, 2023

Codecov Report

Base: 97.34% // Head: 97.35% // Increases project coverage by +0.01% 🎉

Coverage data is based on head (41f6c9f) compared to base (add40b5).
Patch coverage: 100.00% of modified lines in pull request are covered.

❗ Current head 41f6c9f differs from pull request most recent head 966660c. Consider uploading reports for the commit 966660c to get more accurate results

Additional details and impacted files
@@            Coverage Diff             @@
##             main      #53      +/-   ##
==========================================
+ Coverage   97.34%   97.35%   +0.01%     
==========================================
  Files           6        6              
  Lines         602      605       +3     
==========================================
+ Hits          586      589       +3     
  Misses         16       16              
Flag Coverage Δ
unittests 97.35% <100.00%> (+0.01%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
fontbro/font.py 97.18% <100.00%> (+0.01%) ⬆️

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

Copy link
Owner

@fabiocaccamo fabiocaccamo 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 very much for this great PR, just that range object optimization and I merge it.

fontbro/font.py Outdated
@@ -271,17 +271,17 @@ def get_characters(self, ignore_blank=False):
glyfs = font.get("glyf")
for code, char_name in cmap.items():
code_hex = f"{code:04X}"
char = chr(code)
if code in range(0x110000):
Copy link
Owner

Choose a reason for hiding this comment

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

a code_range variable could be created outside the loop to avoid range object creation at every loop iteration.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I changed it to if 0 <= code < 0x110000: which should be cleaner.

@fabiocaccamo fabiocaccamo added bug Something isn't working enhancement New feature or request labels Jan 29, 2023
@fabiocaccamo fabiocaccamo merged commit e6420c9 into fabiocaccamo:main Jan 29, 2023
@seproDev seproDev deleted the fixes branch January 29, 2023 11:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working enhancement New feature or request
Projects
Archived in project
2 participants