-
Notifications
You must be signed in to change notification settings - Fork 85
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 error handling to Request methods #308
base: main
Are you sure you want to change the base?
Add error handling to Request methods #308
Conversation
Make all associated Request methods return a Result<Self, Error> instead of Self and panicking. Change docs and doc-tests to reflect this, and remove manual parsing in doc-tests and unit tests
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.
Few nits, but overall this is a really promising PR. Thank you!
{ | ||
let url = url.try_into().expect("Could not convert into a valid url"); | ||
let url = url.try_into().map_err(|e| Error::new(500, e))?; |
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.
This does not need the map_err
call to work:
let url = url.try_into().map_err(|e| Error::new(500, e))?; | |
let url = url.try_into()?; |
But if it's for clarity purposes, it's preferable to use the Status
trait instead:
let url = url.try_into().map_err(|e| Error::new(500, e))?; | |
let url = url.try_into().status(500)?; |
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.
I got the following error after replacing map_err with status
no method named `status` found for enum `std::result::Result<url::Url, <U as std::convert::TryInto<url::Url>>::Error>` in the current scope
method not found in `std::result::Result<url::Url, <U as std::convert::TryInto<url::Url>>::Error>`
note: the method `status` exists but the following trait bounds were not satisfied:
`<U as std::convert::TryInto<url::Url>>::Error: std::error::Error`
which is required by `std::result::Result<url::Url, <U as std::convert::TryInto<url::Url>>::Error>: status::Status<url::Url, <U as std::convert::TryInto<url::Url>>::Error>`
`<U as std::convert::TryInto<url::Url>>::Error: std::marker::Send`
which is required by `std::result::Result<url::Url, <U as std::convert::TryInto<url::Url>>::Error>: status::Status<url::Url, <U as std::convert::TryInto<url::Url>>::Error>`
`<U as std::convert::TryInto<url::Url>>::Error: std::marker::Sync`
which is required by `std::result::Result<url::Url, <U as std::convert::TryInto<url::Url>>::Error>: status::Status<url::Url, <U as std::convert::TryInto<url::Url>>::Error>`
Make all associated Request methods return a Result<Self, Error> instead of Self and panicking.
Change docs and doc-tests to reflect this, and remove manual parsing in doc-tests and unit tests
Fixes #303