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.

Sign up for GitHub

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

Jump to bottom

Return Result from constructors which take TryInto<Url> #303

Open
yoshuawuyts opened this issue Dec 18, 2020 · 2 comments · May be fixed by #308
Open

Return Result from constructors which take TryInto<Url> #303

yoshuawuyts opened this issue Dec 18, 2020 · 2 comments · May be fixed by #308
Labels
semver-major This change requires a semver major change
Milestone
3.0

Comments

@yoshuawuyts
Copy link
Member

Now that TryFrom<str> for Url has been implemented, using the Url struct is a lot more pleasant. This opens up the possibility again for us to move from a "panic if parsing fails" to returning errors if parsing fails.

// current, panic if the url is malformed
let req = Request::post("https://api.foo.com/berries");

// proposed, throw an error instead
let req = Request::post("https://api.foo.com/berries")?;
@yoshuawuyts yoshuawuyts added the semver-major This change requires a semver major change label Dec 18, 2020
@yoshuawuyts yoshuawuyts added this to the 3.0 milestone Dec 18, 2020
@brightly-salty
Copy link
Contributor

I'd be willing to attempt to implement this and submit a PR. Are you suggesting using something like the thiserror crate, or a custom error type with a custom std::error::Error impl?

@yoshuawuyts
Copy link
Member Author

that's a good question; I think this should wrap the url::Url's error with the Status trait and send back a 500 status code. We don't need a custom impl or other crate since http-types already has its own Error type.

@brightly-salty brightly-salty linked a pull request Dec 19, 2020 that will close this issue
Open
@Fishrock123 Fishrock123 mentioned this issue May 17, 2021
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
semver-major This change requires a semver major change
Projects
None yet
Milestone
3.0
Development

Successfully merging a pull request may close this issue.

Add error handling to Request methods brightly-salty/http-types
2 participants
@yoshuawuyts @brightly-salty