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

Clean up and re-structure the errors #137

Closed
marioortizmanero opened this issue Oct 11, 2020 · 1 comment · Fixed by #258
Closed

Clean up and re-structure the errors #137

marioortizmanero opened this issue Oct 11, 2020 · 1 comment · Fixed by #258
Labels
discussion Some discussion is required before a decision is made or something is implemented help wanted Extra attention is needed
Milestone

Comments

@marioortizmanero
Copy link
Collaborator

The ClientError type currently defined for the next release could definitely be improved. Some of its variants are repetitive and confusing. I kept adding variants for the rewrite without thinking much about the structure because the important thing was to get the rewrite done. But I would like to discuss what it should be like.

Here's more or less the current definition:

#[derive(Debug, Error)]
pub enum ClientError {
    #[error("invalid client authentication: {0}")]
    InvalidAuth(String),

    #[error("request unauthorized")]
    Unauthorized,

    #[error("exceeded request limit")]
    RateLimited(Option<usize>),

    #[error("request error: {0}")]
    Request(String),

    #[error("status code {0}: {1}")]
    StatusCode(u16, String),

    #[error("spotify error: {0}")]
    API(#[from] APIError),

    #[error("json parse error: {0}")]
    ParseJSON(#[from] serde_json::Error),

    #[error("url parse error: {0}")]
    ParseURL(#[from] url::ParseError),

    #[error("input/output error: {0}")]
    IO(#[from] std::io::Error),

    #[cfg(feature = "cli")]
    #[error("cli error: {0}")]
    CLI(String),

    #[error("cache file error: {0}")]
    CacheFile(String),
}

Some thoughts:

  • StatusCode, Request, RateLimited and Unauthorized could be wrapped under the same variant. These are just possible HTTP errors, so something like HTTPError could do fine. I understand that having Unauthorized and such can be useful to match against these specific errors, but if HTTPError was defined properly it wouldn't be necessary (say, using http::status::StatusCode, for example).
  • Maybe CLI should be renamed to UserError? It describes better its purpose.
  • Is CacheFile necessary? I don't see it used anywhere, and it could be replaced with IO.
  • Should ParseURL and ParseJSON be wrapped under a Parse variant? Maybe that's unnecessary?
  • Should InvalidAuth be renamed to something different? It might be confusing when compared to Unauthorized, but InvalidAuth means that Spotify wasn't initialized correctly (say, there's no token when it's needed). Maybe MissingAuth? IDK
  • None of these errors are documented (with ///). They should include a comment with a brief description.

Please let me know any other problems you see and if you'd like to change something else.

@marioortizmanero marioortizmanero added help wanted Extra attention is needed discussion Some discussion is required before a decision is made or something is implemented labels Oct 11, 2020
@hrkfdn
Copy link
Contributor

hrkfdn commented Oct 18, 2020

Hey,

Some very reasonable points.

  • StatusCode, Request, RateLimited and Unauthorized could be wrapped under the same variant. These are just possible HTTP errors, so something like HTTPError could do fine. I understand that having Unauthorized and such can be useful to match against these specific errors, but if HTTPError was defined properly it wouldn't be necessary (say, using http::status::StatusCode, for example).

Something I'd like to point out: The RateLimited enum variant also contains the duration to wait until the new request can be made (taken from Retry-After header, see https://developer.spotify.com/documentation/web-api/#rate-limiting). It would be nice if that's still accessible with the refactorings.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
discussion Some discussion is required before a decision is made or something is implemented help wanted Extra attention is needed
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants