-
Notifications
You must be signed in to change notification settings - Fork 118
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
HTTP Error cleanup #258
HTTP Error cleanup #258
Conversation
if response.status().is_success() { | ||
response.text().await.map_err(Into::into) | ||
} else { | ||
Err(HttpError::from_response(response).await) | ||
Err(ReqwestError::StatusCode(response)) |
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.
It seems you don't show how to construct the ReqwestError::Client()
? When will then client return this error type?
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.
It's just the same as reqwest::Error
, so whenever that happens. For example when making the request or when converting the reply into a String.
Let me know if you like the updated documentation! This can be merged if you're ok with it. |
rspotify-http/Cargo.toml
Outdated
@@ -14,6 +14,8 @@ keywords = ["spotify", "api"] | |||
edition = "2018" | |||
|
|||
[dependencies] | |||
rspotify-model = { path = "../rspotify-model", version = "0.10.0" } |
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.
Is it necessary to include rspotify-model
?
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.
Yes, because it's used in examples in the documentation
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.
Oh sorry! it should be in dev-dependencies. Fixed.
The CI flow failed :) |
Fixed! |
Description
This cleans up the errors for
rspotify-http
, according to #137.I would say the rest of the error types are OK, so this closes #137.
Motivation and Context
The current error type for http tries to wrap up all the errors from the clients we support under the same interface. However, it's a mess because:
HttpError::Io
is only needed for ureqRateLimited
orUnauthorized
but it'sStatusCode
for the rest?Api
andStatusCode
variants the same?So this PR attempts to fix it by simply having a variant of error for each client supported by
rspotify-http
. It's as easy as delegating error handling to the HTTP client. If the user has enabledureq
, then they should deal withureq::Error
instead of some wrapper of our own that might not have everything they need.This way, error handling is complete. There is nothing of functionality we miss by wrapping all of them under the same interface. And we also don't have to maintain such a complex wrapper ourselves.
I remember that @hrkfdn pointed out that the
RateLimited
variant was a must for him in order to retry a request. Well, this design still makes it possible to do so. It's just dependent on what HTTP client he's using. If he uses reqwest then it should be something like:And if someone wanted to get the API error from Spotify:
Dependencies
None
Type of change
Please delete options that are not relevant.
How Has This Been Tested?
The CI still passes