-
-
Notifications
You must be signed in to change notification settings - Fork 119
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
Replace some of the unwrap() calls #308
base: master
Are you sure you want to change the base?
Conversation
*Also remove clippy errors
Remove a todo that was added in previous commit
src/errors.rs
Outdated
Error::Ssl(ref err) => err.fmt(f), | ||
Error::Mime(ref err) => err.fmt(f), |
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.
Instead of printing the source error, we could also return it in StdError::source
and either print nothing here or a message "on top" of what happened.
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.
Made two new commits. Not really sure if second commit was necessary (commit f1e58c), please check it and i will undo it if needed. The problem with it is that you said that i could either "print nothing here" (Which is definitely not an option) or print a message "on top" of what happened, which will output something like this with Mime type: Mime error: mime parse error: <ParseError>
(See how mime::FromStrError displays itself at its source code). As you can see it does not look really good, first two statements just repeat each other. I would remove this "Mime error:" at the beginning and leave just err.fmt(f)
in code how it was before. If i'm wrong and i undo these changes, i might need to redo them again, so just wanna see what you think about that before doing anything
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.
If we don't have anything more meaningful to print on top of the original error, why is printing nothing not an option? Together with returning the source
, libraries like anyhow
will print the entire chain of errors and thus print the "inner" error.
But, I think the better option would be to move away from a general "Ssl" error and be more specific of what failed. I commented inline with some ideas, feel free to ignore if you find them too verbose :)
Add Error::Ssl and Error::Mime to StdError::source implementation of Error
Add error messages on top of Error::Ssl and Error::Mime messages to explain what the error is about
src/container.rs
Outdated
let mime = "application/x-tar".parse::<Mime>().unwrap(); | ||
let mime = "application/x-tar".parse::<Mime>()?; |
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.
Can this actually ever fail? unwrap
/expect
seems to be ok here.
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.
Yes, this is right. Thanks for your advice a lot
connector | ||
.set_private_key_file(&Path::new(key), SslFiletype::PEM) | ||
.unwrap(); | ||
connector.set_certificate_file(&Path::new(cert), SslFiletype::PEM)?; |
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.
Instead of the generic Ssl error, we could map to a variant Error::InvalidCertificate
. That would give us something meaningful to say in our Display
impl without having to print the inner error.
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.
SSL error can be returned not just here, it will be generated, for example in docker.rs at line 54: let mut connector = SslConnector::builder(SslMethod::tls())?;
, and on line 59: connector.set_private_key_file(&Path::new(key), SslFiletype::PEM)?;
(And there are more). So we can't really guarantee that all of these errors will be caused by an invalid certificate, and in my opinion, it's better to make just a general SSL error, containing an inner error
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.
Yes, we would need more than one variant, not just InvalidCertificate
but also InvalidPrivateKey
etc.
Which is why I said "feel free to ignore if you think it becomes too verbose" :)
A general Ssl error is certainly more concise but the error messages will be less helpful because it is not as clear where the error originated.
What did you implement:
Replace some of "unwrap()" calls either with a question mark "?" or a match statement (Had to change return type of some functions (Didn't do anything to public functions/methods) and add new variants and traits for Error enum (See the changelog section)).
Also fix all clippy warnings.
Notes:
std::io::Result
return type instead of built-in Result type in this crate. This is because i would have to implement something like JSON variant for the Error enum, then add different kinds for JSON errors as well, and increase complexity of code. This return type just works, so i used it insteadPartially fixes issue #301 (There are still many unwrap calls outside of tests and examples)
How did you verify your change:
Building and testing the project was successful
What (if anything) would need to be called out in the CHANGELOG for the next release:
(This is probably not important, i just listed all public changes)
Added new variant for the Error enum: Ssl, implemented trait
From<openssl::error::ErrorStack>
for Error enum. Also modified implementation of StdError trait for Error, it can now return source of its new variant