Remove (or annotate) unwrap() from fontdrasil #1880
Conversation
simoncozens
force-pushed
the
safe-fontdrasil
branch
2 times, most recently
from
818bbc3 to
ee99af4
Compare
simoncozens
force-pushed
the
safe-fontdrasil
branch
from
ee99af4 to
e7d7b3c
Compare
cmyr
left a comment
cmyr
left a comment
There was a problem hiding this comment.
looks good simon, thanks for taking this on!
| if let Some(weight) = weights.last() { | ||
| trace!(" {} {:?}", loc_idx, weight); | ||
| } |
There was a problem hiding this comment.
alternatively we could just remove the unwrap, and we will debug-print the Option.
| assert!( | ||
| minmax.contains_key(tag), | ||
| "axis_order contained axes not in locations" | ||
| ); | ||
| #[allow(clippy::unwrap_used)] |
There was a problem hiding this comment.
totally not necessary to change but: this could also be expressed a bit more concisely by just using expect in place of unwrap below, with the same text as your assertion here.
There was a problem hiding this comment.
True, but I see a philosophical difference. I want to ban expects because they're just unwraps with an error message. I know the actual outcome is the same, but assert affirmatively signals that we're testing an internal invariant ("In a mad world this might fail, let's make sure and bug out if it does"), while .unwrap/.expect is "I am positively sure this won't fail".
Co-authored-by: Colin Rofls <colin@cmyr.net>
simoncozens
force-pushed
the
safe-fontdrasil
branch
from
b408c36 to
348c336
Compare
2 participants
This PR bans
unwrap,expectand indexing slicing (i.e. panicable operations) from fontdrasil.unwrap()is replaced either with aok_oror explained with an annotation.This does change the API somewhat; creating coordinate convertors, and converting coordinates, returns a
Result.There is also an unrelated change to the version of
vergen-gitclused by fontc. Without this, we were usingvergen-gitcl1.0 which pulled invergen v9.1.0, which had a very different API to the onevergen-gitclexpected. So I couldn't get things to compile without it.Fixes #1879