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

Try to make field matching in fontra more obvious with newtypes. Don't reinvent chunks. #711

Merged
merged 1 commit into from
Feb 7, 2024

Conversation

rsheeter
Copy link
Contributor

@rsheeter rsheeter commented Feb 6, 2024

Followon to #701

@rsheeter rsheeter changed the title Try to make field matching in fontra match better. Don't reinvent chunks. Try to make field matching in fontra more obvious with newtypes. Don't reinvent chunks. Feb 6, 2024
Copy link
Member

@dfrg dfrg left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Member

@anthrotype anthrotype left a 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?

Copy link
Member

@cmyr cmyr left a 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.

@rsheeter
Copy link
Contributor Author

rsheeter commented Feb 7, 2024

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.

@anthrotype
Copy link
Member

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.

String is certainly not a Tag. And both a layer name and an axis name, unlike a tag, don't have any particular requirements beyond being a string. So it would be correct to type both as a String in my opinion. You can argue that the python code you were looking at was ambiguous in that it seemed to be using str type for an axis tag (which it wasn't in the end).

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.

@rsheeter
Copy link
Contributor Author

rsheeter commented Feb 7, 2024

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 Location = dict[str, float] without a comment. What's the key? - maybe you know from convention it's likely to be a name but if you aren't aware of that convention it's not at all obvious. If it said Location = dict[AxisName, float] or had a comment it would be more clear. I hope using the type aliases - I agree they don't need to be newtypes - is similarly helpful. If not ... well, it probably doesn't do any harm as far as I can tell.

@rsheeter rsheeter added this pull request to the merge queue Feb 7, 2024
Merged via the queue into main with commit 7156651 Feb 7, 2024
10 checks passed
@rsheeter rsheeter deleted the g3 branch February 7, 2024 17:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants