-
Notifications
You must be signed in to change notification settings - Fork 15
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
Try to make field matching in fontra more obvious with newtypes. Don't reinvent chunks. #711
Conversation
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.
LGTM
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'm not entirely sure what exactly this buys us besides making the use of these strings slightly more verbose. Do AxisName or LayerName differ in behavior from each other or from a regular string?
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'm with cosimo here, I'm not sure this is buying us anything.
I feel strongly that leaving it as naked String is misleading and error prone. There is at least one spot in fontra already where pythong declares str, no comment, and I had to look at actual data to see if it was a tag an axis name. I'm not especially tied to using a new type so I will update to type aliases. |
Also, by the way, the two type aliases are just that, alternative names to the same type, which is still String. One could still use a LayerName or an AxisName (or String) interchangeably and nothing would happen -- and that's fine. |
The goal is merely it's a little bit clearer what matches what. You can still do the wrong thing but perhaps it's a little bit less likely. For example, in python I see |
Followon to #701