Build Better Errors #130
Build Better Errors #130
Conversation
src/errors.rs
Outdated
| "no transports were configured in the authenticator service" | ||
| ), | ||
| AuthenticatorError::Platform => write!(f, "unknown platform error"), | ||
| AuthenticatorError::InternalError(ref err) => write!(f, "internal error: {:?}", err), |
There was a problem hiding this comment.
Any specific reason why we're using Debug here and not Display for a string? (I.e. {:?} vs {})
There was a problem hiding this comment.
No reason, changed! Thanks :)
src/errors.rs
Outdated
|
|
||
| impl std::fmt::Display for U2FTokenError { | ||
| fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { | ||
| write!(f, "{:?}", self) |
There was a problem hiding this comment.
Why Debug and not as_str? Alternatively, why is as_str private but never used?
There was a problem hiding this comment.
ahaha, I fully intended to come back to this, thank you! Changing to as_str as intended, and now it's not dead code. 🎉
src/manager.rs
Outdated
| self.tx | ||
| .send(QueueAction::Cancel) | ||
| .map_err(|_| crate::Error::Unknown) | ||
| .map_err(|e| AuthenticatorError::from(e)) |
There was a problem hiding this comment.
Isn't this from call what ? does already? (see https://doc.rust-lang.org/std/convert/trait.From.html#examples). Something like Ok(self.tx.send(...)?) is terser, unless you think the explicit call to from is worth keeping.
(Though I think ideally this would look like self.tx.send(...).into() but it seems the necessary impl to make this work isn't in std)
There was a problem hiding this comment.
Aha, I didn't even look at this closely, thank you!
jcjones
force-pushed
the
better_errors
branch
from
ac57247 to
db4ac5e
Compare
Major changes: * Move to Rust 2018 edition #125 * Remove dependency on boxfnonce #121 * Reworked error handling #130 * Added a higher-level AuthenticatorService that can use multiple backends #123 * Changed the C API to use the new AuthenticatorService #123 * Added a Status channel for backends #122 * Now obtaining HID report sizes from the descriptors #112 * Add authenticator USB and Firmware details to the C API #93
As-is, everything comes down to the 5 enumerated U2F errors, yet the library needs to handle U2F and CTAP2, as well as other miscellaneous things that may be problematic. And it needs to do so while also smoothly passing along U2F errors to the C API, when they occur.
This PR refactors all the errors to use a main error type located in
crate::errors, and a typedef forcrate::Resultfor all result objects. It has some convenience functions for handling different error types, and is intended to be extended as we add the necessary CTAP2 and crypto pieces.I am not super happy with the verbosity of what I've put together, particularly code like:
but honestly, that's not that hard to refactor compared to the rest of this, and only really affects
statemachine.rsbadly, so in the interests of keeping things moving along, I'm going to go ahead and request review of this.I am totally open to ideas for how to make the semantics of the above better. Obviously direct imports to avoid the
errors::prefixes would help a slight amount, but maybe it's a call for something more like:or even have the method return the
Err(...)so it's justcallback.call(errors::u2f(...))?I could also see shortening U2FTokenError.
Anyway, open to suggestions of any manner. Thanks!