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

Add UFO dir and mandatory .plist file read path existence checks and invalid path diagnostics #143

Conversation

chrissimpkins
Copy link
Collaborator

@chrissimpkins chrissimpkins commented Jul 20, 2021

Current formatted error:

UFO load failed with error: PlistError(Error { inner: ErrorImpl { kind: Io(Os { code: 2, kind: NotFound, message: "No such file or directory" }), file_position: None } })

New formatted error with changes in this PR:

UFO load failed with error: invalid path: "bogus/path"

Tested in this executable with argument bogus/path:

fn main() {
    for arg in std::env::args().skip(1) {
        let mut ufo = match norad::Font::load(&arg) {
            Ok(u) => u,
            Err(e) => {
                eprintln!("UFO load failed with error: {}", e);
                std::process::exit(1);
            }
        };
    }
}

@chrissimpkins chrissimpkins changed the title Add UFO dir read path existence check and invalid file path diagnostics Add UFO dir read path existence check and invalid path diagnostics Jul 20, 2021
@madig
Copy link
Collaborator

madig commented Jul 22, 2021

Ah, yes, I'm interested in better error messages. I think this can be done more elegantly maybe, though. One could match on plist::from__file and if it returns an Io error, return that one instead of the generic PlistError. Not sure if one can implement a more user friendly user-facing error string on it though 🤔

Edit: there's also this method on plist's Error type: https://docs.rs/plist/1.1.0/plist/struct.Error.html#method.is_io, so it's easy to determine if it's actually an IO error.

@chrissimpkins chrissimpkins changed the title Add UFO dir read path existence check and invalid path diagnostics Add UFO dir and mandatory .plist file read path existence check and invalid path diagnostics Jul 22, 2021
@chrissimpkins chrissimpkins changed the title Add UFO dir and mandatory .plist file read path existence check and invalid path diagnostics Add UFO dir and mandatory .plist file read path existence checks and invalid path diagnostics Jul 23, 2021
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.

Thanks chris, improving diagnostics is always helpful!

@cmyr
Copy link
Member

cmyr commented Jul 26, 2021

@madig do you have further thoughts on this? I'm happy to merge as-is, I think there are other possible approaches but this one feels reasonable. 🤷

@chrissimpkins
Copy link
Collaborator Author

@madig do you have further thoughts on this? I'm happy to merge as-is, I think there are other possible approaches but this one feels reasonable. 🤷

Let me know if you'd like me to pull d9219be out and address the mandatory file validations in another PR

@cmyr
Copy link
Member

cmyr commented Jul 27, 2021

Thinking about this a bit more, what I would love is a new MissingFile error variant that specifically handles the case of a missing mandatory file; it can have a field where you pass a string representing the file in question ("metainfo.plist", "background/contents.plist"). So maybe a PR that adds that variant along with the check for metainfo.plist, using that variant, would be a good start?

@chrissimpkins
Copy link
Collaborator Author

Wrap the UFO dir check into that error handling approach and close this PR?

@chrissimpkins
Copy link
Collaborator Author

Thinking about this a bit more, what I would love is a new MissingFile error variant that specifically handles the case of a missing mandatory file; it can have a field where you pass a string representing the file in question ("metainfo.plist", "background/contents.plist"). So maybe a PR that adds that variant along with the check for metainfo.plist, using that variant, would be a good start?

How about this approach instead? #146

@chrissimpkins chrissimpkins deleted the invalid-ufo-path-diagnostics branch August 20, 2021 22:10
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.

3 participants