[GH-ISSUE #137] Clean up and re-structure the errors #50

New issue

Closed

opened 2026-02-27 20:22:48 +03:00 by kerem · 1 comment

kerem commented

2026-02-27 20:22:48 +03:00

Owner

Originally created by @marioortizmanero on GitHub (Oct 11, 2020).
Original GitHub issue: https://github.com/ramsayleung/rspotify/issues/137

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.

Originally created by @marioortizmanero on GitHub (Oct 11, 2020). Original GitHub issue: https://github.com/ramsayleung/rspotify/issues/137 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: ```rust #[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`](https://docs.rs/http/0.2.1/http/status/struct.StatusCode.html), 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.

kerem

2026-02-27 20:22:48 +03:00

closed this issue
added the
help wanted

discussion
labels

kerem commented

2026-02-27 20:22:50 +03:00

Author

Owner

@hrkfdn commented on GitHub (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.

@hrkfdn commented on GitHub (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.

kerem referenced this issue

2026-02-27 20:23:47 +03:00

[PR #50] [MERGED] Fixes #49 #209