search

yeslogic / allsorts

Font parser, shaping engine, and subsetter implemented in Rust
https://yeslogic.com/blog/allsorts-rust-font-shaping-engine/
Apache License 2.0
706 stars 23 forks source link

Derive more things #69

Closed LoganDark closed 2 years ago

LoganDark commented 2 years ago

I find myself running into unimplemented traits all the time, here are the main offenders. Especially important are Info and GlyphPosition

Let me know if any of the defaults I've picked are inappropriate.

(Also PR 69; nice)

LoganDark commented 2 years ago

I went ahead and added #[non_exhaustive] to the error types to make adding new variants a non-breaking change. That makes this PR a breaking change, but it's good for API stability long-term. (It prevents users from using match without a catch-all _ => case.)

You may also want to look into seeing which structs could also be made #[non_exhaustive], which prevents downstream users from constructing them, so that adding new (public or non-public) fields is a non-breaking change as well.

LoganDark commented 2 years ago

Checks have passed

LoganDark commented 2 years ago

Looks like that Default was actually needed by allsorts, I didn't add it in this PR so maybe I shouldn't remove it either

wezm commented 2 years ago

Oh well this is embarrassing, looks like that Default impl I asked you to remove was there prior to your changes and the code was relying on it. Feel free to drop that last commit and accept my apologies for the run around.

LoganDark commented 2 years ago

Checks have already passed so merge when ready

wezm commented 2 years ago

I merged this and #71 manually. In the end I decided to drop non_exhaustive from the error types. I think adding this attribute makes sense when a project has reached a 1.0 release but until then I think it's better to get an error about an unhanded variant if a new one is added, versus it silently being swallowed by a catch all branch in a match.