-
Notifications
You must be signed in to change notification settings - Fork 448
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
Unify pen argument names #2919
Open
jenskutilek
wants to merge
22
commits into
main
Choose a base branch
from
unify-pen-argnames
base: main
Could not load branches
Branch not found: {{ refName }}
Could not load tags
Nothing to show
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
Open
Unify pen argument names #2919
Changes from 4 commits
Commits
Show all changes
22 commits
Select commit
Hold shift + click to select a range
4cac72d
Annotate types for all SegmentPen.addComponent methods
jenskutilek eb63e3b
Unify argument names for PointPens
jenskutilek cd40cc5
Annotate addPoint method defs
jenskutilek 8715b5b
Add annotations in hashPointPen
jenskutilek 923b750
Fix indentation
jenskutilek efc0a47
Fix method call
jenskutilek 9d582f8
Fix indentation
jenskutilek d033a79
Add annotations to basePen
jenskutilek 7fb9bad
Indentation
jenskutilek 8b6e6b6
Add annotations to areaPen
jenskutilek 9ab26a9
Add annotations to teePen
jenskutilek 4142276
Add blank lines
jenskutilek fe5ba52
Add annotations to pointPen
jenskutilek aa73997
Add annotations to filterPen
jenskutilek 9366b9b
Add annotations to cairoPen
jenskutilek 1b8546c
Add annotations to cocaPen
jenskutilek 7bf1edd
Add annotations to cu2quPen
jenskutilek 3968435
filterPen: process missing identifiers
jenskutilek dd83889
Add annotations to transformPen (incomplete)
jenskutilek 21f72cb
Undo transformPen
jenskutilek e87f818
Add TypeAlias for PenGlyphSet
jenskutilek 1688400
Add missing arg
jenskutilek File filter
Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.
mixing tabs and spaces?
feels weird we add typing only to this method and not the rest. Maybe for now just rename the parameters if you don't have time to add type hints to all.
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.
C&P error ...
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.
I've started to add typing annotations to the pens. But I had to stop now, because it gets complicated and even requires some code rewriting to pass the type checks. But at least there is more typing than before now :)
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.
Hm, where would it require code rewriting?
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.
I needed to add some assertions, otherwise the type checker would complain about variables possibly being None, e.g.:
https://github.com/fonttools/fonttools/pull/2919/files#diff-713c63d7dd345e61c8c06754b31811b60345f57b1b2c70fdf7bb8aaf7227c92aR408-R420