feat: Improved error handling by using thiserror
#208
Conversation
|
Thanks! I'm not opposed to the idea, though I'm curious how much this adds to build time — could you measure that ( |
OtaK
force-pushed
the
feat/error-handling
branch
from
a6543a9 to
8bbe9c6
Compare
|
Sure - I made sure to run Also rebased this branch on top of main so it should be up to date Main branchCargo Build Timings — fantoccini 0.20.0-rc.4-main.pdf This branchCargo Build Timings — fantoccini 0.20.0-rc.4-feat-error-handling.pdf |
| url = "2.2.2" | ||
| serde = { version = "1.0.103", features = ["derive"] } | ||
| serde_json = "1.0.25" | ||
| url = "2.2" | ||
| serde = { version = "1.0", features = ["derive"] } | ||
| serde_json = "1.0" |
There was a problem hiding this comment.
I'd like to avoid these changes — the minimal listed patch versions are often there for a reason too.
| pub enum NewSessionError { | ||
| /// The given WebDriver URL is invalid. | ||
| BadWebdriverUrl(ParseError), | ||
| #[error("webdriver url is invalid: {0}")] | ||
| BadWebdriverUrl(#[from] ParseError), |
There was a problem hiding this comment.
Let's use #[source] instead of #[from] for any of these we don't already have an explicit impl From for. It's not always the case that a public impl From makes sense, especially not a single one.
| pub enum NewSessionError { | ||
| /// The given WebDriver URL is invalid. | ||
| BadWebdriverUrl(ParseError), | ||
| #[error("webdriver url is invalid: {0}")] |
There was a problem hiding this comment.
I think the general guidance from the error working group is that you either print the inner error in the message or you propagate it using Error::source (which both #[from] and #[source] do I believe). This currently does both. Let's remove : {0} here (and for all the others with #[from]/#[source]) and stick with just exposing source.
| UnknownMethod => StatusCode::METHOD_NOT_ALLOWED, | ||
| UnknownPath => StatusCode::NOT_FOUND, | ||
| UnsupportedOperation => StatusCode::INTERNAL_SERVER_ERROR, | ||
| Self::ElementClickIntercepted |
There was a problem hiding this comment.
I'd prefer to make this change in another PR.
|
Huh, that's surprising — usually extra proc macros add a decent amount to build time even just because |
No idea if you would like this to be in; Seems to simplify things a lot when it comes to implementing & extending errors.
My branch is (very) out of date compared to the
mainbranch. I'll get to rebasing it soon.