-
Notifications
You must be signed in to change notification settings - Fork 13
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
[fontbe] pass explicit axis_count when constructing Gvar #833
Conversation
Fixes #815 Untested as it requires googlefonts/fontations#919 (and write-fonts dependency bump) to work...
// in the unlikely event that we have more than 65535 axes... | ||
.map_err(|_| Error::OutOfBounds { | ||
what: "axis count".into(), | ||
value: axis_order.len().to_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 would personally unwrap here, since I can't imagine this being possible for any conceivable font, but 🤷
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.
just wait a few years :)
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 strongly prefer Result unless it can't happen. Rationale: if we unwrap all the things that shouldn't happen but plausibly can then it's all but assured we'll hit some of them in production and rue the day we didn't use Result.
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.
looks good!
f3c7154
to
9fc1180
Compare
Will fix #815
CI will be 🔴 until googlefonts/fontations#919 is merged write-fonts dependency bumped