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 error handling to Request methods #308

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

brightly-salty
Copy link
Contributor

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

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
Copy link
Member

@yoshuawuyts yoshuawuyts left a 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))?;
Copy link
Member

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:

Suggested change
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:

Suggested change
let url = url.try_into().map_err(|e| Error::new(500, e))?;
let url = url.try_into().status(500)?;

Copy link
Contributor Author

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>`

src/request.rs Show resolved Hide resolved
@Fishrock123 Fishrock123 added the semver-major This change requires a semver major change label Aug 5, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
semver-major This change requires a semver major change
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Return Result from constructors which take TryInto<Url>
3 participants