Add custom missing mandatory UFO path error types & missing mandatory UFO path checks

[chrissimpkins]

Supersedes #143

New approach based on suggestion in #143 (comment)

TODO:

• add metainfo.plist check (d9d26c4)
• add glyphs/contents.plist check (a418ef2)
• add tests for all new missing mandatory path checks

[cmyr]

Looks good, thanks!

[chrissimpkins]

I'll wrap up the items in the TODO list and then convert out of Draft for a full implementation review. If this approach seems reasonable, this should get us to read validations / error handling on all required UFO paths

[chrissimpkins]

This PR now includes:

• UFO directory path existence validation check and custom norad error
• metainfo.plist @ root validation check and custom norad error
• contents.plist in glyphs / background glyphs layer dir check and custom norad error
• layercontents.plist @ root (if UFOv3 source) check error handling refactor with different custom norad error (same as other missing file errors)
• new test sources & tests for each UFO spec mandatory file violation check

Ready for review.

[madig]

assert_eq!(font_load_res, Err(Error::MissingUfoDir(...))) or something maybe?

[chrissimpkins]

Tried. Compiler won't let me perform equality comparisons on the Result Error type

[cmyr]

Two things:

As written, I would do this as,

assert!(matches!(font_load_res, Err(Error::MissingUfoDir(_))));

but there's probably a better option: explicitly testing for a panic with the should_panic attribute.

[chrissimpkins]

but there's probably a better option: explicitly testing for a panic with the should_panic attribute.

Ah nice, I didn't realize that you can add an expected string with should_panic. I tried this approach but I can't elicit a panic from the new custom Error. The attribute doesn't seem to catch errors like it does panics. I see this in the docs that you linked:

You can’t use the #[should_panic] annotation on tests that use Result<T, E>. Instead, you should return an Err value directly when the test should fail.

Let me tinker with the test Result approach a bit. If I can't find a better solution, I'll transition things over the matches! macro approach above.

[cmyr]

Maybe it would work if you just unwrap the Result?

[cmyr]

(but matches! is totally reasonable too)

[chrissimpkins]

It looks like the use of a Result return type in unit tests is good if you expect an Ok() but there isn't a way to expect an Err. We could return a test error when the function being tested returns Ok 💥 but that seems worse than the match pattern matching approach :)

I went with the matches! macro. That allows us to confirm the custom error type and works well!

b1c17ab

[madig]

Maybe consider removing inessential files from the test UFOs, like lib.plist.

[chrissimpkins]

Maybe consider removing inessential files from the test UFOs, like lib.plist.

a25963b

[cmyr]

This looks good to me, feel free to merge at your leisure @chrissimpkins.

[chrissimpkins]

This looks good to me, feel free to merge at your leisure @chrissimpkins.

Thank you both for all of the feedback here!