[GH-ISSUE #127] Meta-Issue

kerem commented

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

Owner

Originally created by @Kestrer on GitHub (Sep 13, 2020).
Original GitHub issue: https://github.com/ramsayleung/rspotify/issues/127

This is a meta-issue of many issues with rspotify. Some of it's opinionated but most of it I think is just an improvement.

General

#173 The crate root contains nothing other than modules. This means that paths get very long and verbose (e.g. rspotify::client::Spotify). The entire contents of the client module should be moved to the crate root to make it easier to access.
#128 senum; this module contains all the enums used (and also Error for some reason), but could be split into more fitting modules such as model.
#129 Rspotify is designed for use from a CLI. Lots of the functions read from stdin, but this is out of scope for this library. Either they should be removed or contained in a cli/tui/terminal/console module and feature-gated.
#129 This library extensively uses a fake builder pattern - it's like the real builder pattern but it's all the same type. To add proper type safety the builder patterns should be properly implemented in their own types.
#128 Many functions like Spotify::tracks take in a Vec<&str>/Vec<String>/&[String]/&[&str], when they should take in an impl IntoIterator<Item = &str>.
#128 Some endpoints and functions in Rspotify take String as parameters instead of &str.
~~limit and offset parameters should universally be usizes as they are easier to work with from Rust.~~
#129 Actual Default::default impl (see TokenInfo::default)
#129 Spotify::get_uri should useformat!:

    fn get_uri(&self, _type: Type, _id: &str) -> String {
        format!("spotify:{}:{}", _type.as_str(), self.get_id(_type, _id))
    }

#129 Fix the documentation links to Spotify endpoints and make the format more consistent
#161 Make a separate type for IDs parsed by Spotify::get_id
#226 Authenticated tests should actually not modify the user's account and cause minimal changes (as in, don't completely delete the user's library for a test).

OAuth

#185 TokenInfo stores expires_in as a u32, and additionally has the expires_at field. This is not easy to use, instead it should have a single expires field with the type Instant.
#185 TokenInfo stores a scope: String, when it should store a scope: Vec<String> obtained by splitting the scopes by spaces.
#129 TokenInfo stores token_type, which is useless since it is always Bearer.
#129 The SpotifyClientCredentials and SpotifyOAuth have a superfluous Spotify prefix; just ClientCredentials would be shorter and not duplicate the crate name.
#129 SpotifyClientCredentials contains not only the user's client credentials but also the token info if present, so the name is misleading. In fact the SpotifyOAuth struct duplicates the client_id and client_secret fields due to the lack of a real ClientCredentials type.
#129 A new Client is created every time fetch_access_token is called. This is very inefficient. Instead, the authorization logic should be moved to Spotify allowing it to reuse the same client.
#129 As far as I can tell token_info is never mutated on SpotifyClientCredentials. This means that once the initial token has expired rspotify will get a new token every time. The solution is to wrap TokenInfo into a Mutex and mutate it when appropriate.
#129 Spotify contains an access_token twice; once in the struct itself and once in client_credentials_manager.token_info_access_token.

Utils

#128 convert_map_to_string is better served by serde_urlencoded or even better Reqwest's query method.
A few of these functions like generate_random_string should be private.
#185 datetime_to_timestamp is only necessary due to this library's use of numbers instead of Instants or Durations.
#129 The token functions should probably be methods of Spotify instead of standalone functions.
#185 If all the above suggestions are incorporated the utils module can be removed entirely, and any remaining functions can simply be placed in the crate root for easy access.

Model

#128 The model module is not reexported from the crate root. Having pub use model::* and pub use {album, artist, audio, etc}::* would keep the docs uncluttered and allow users to write rspotify::FullAlbum instead of the overly verbose rspotify::model::album::FullAlbum.
#145 Derive PartialEq (and Eq if possible) for everything.
#244 The _type fields on the models are unnecessary data. It would be better to use a macro to create constants that can only serialize to and deserialize as their string, for example TypeAlbum which is represented in JSON as only "album". This avoids carrying extra data while still storing the type field.
~~The uri and href fields are unnecessary, since it can be recreated easily. Make these methods instead.~~
#145 copyrights is represented as a Vec<HashMap<String, String>>; a much more accurate representation is Vec<Copyright> and

struct Copyright {
    text: String,
    r#type: CopyrightType,
}
enum CopyrightType {
    P,
    C,
}

#145 FullAlbum::release_date and FullAlbum::release_date_precision are Strings. Instead, it should be deserialized into a proper date type like NaiveDate and a enum DatePrecision { Year, Month, Day }.
#145 The same applies to SimplifiedEpisode and FullEpisode.
#157 Types like FullAlbums, PageSimplifiedAlbums, etc exist as wrappers. This makes the API more complex. Instead, have that type exist solely in the API function that needs it, and then return a Vec<FullAlbum>. This drastically reduces the number of public API types.
#145 Restrictions is located in the album module, despite it not having any particular relevance to albums.
#145 {FullArtist, FullPlaylist}::followers is a HashMap<String, Option<Value>>. It should be a struct Followers { total: usize }.
#177 AudioAnalysisSection::mode and AudioFeatures::mode are f32s but should be Option<Mode>s where enum Mode { Major, Minor } as it is more useful.
#145 This is more opinionated but AudioAnalysisSection and AudioAnalysisSegment could contain a #[serde::flatten]ed TimeInterval instead of the having the fields inline.
#145 AudioAnalysisMeasure is identical to TimeInterval, and so should be replaced by it.
~~AudioFeatures::analysis_url and AudioFeatures::track_href aren't useful for the user, since they will access it via this library.~~(The assumption about how do users use this library is not accurate, since we have no idea about how do users use this library)
#157 AudioFeatures::duration_ms should be replaced with a duration of type Duration.
~~[ ] Context contains a uri, but since those aren't particularly useful it should be replaced with a transformer into an id.~~
#145 Actions::disallows can be replaced with a Vec<DisallowKey> or HashSet<DisallowKey> by removing all entires whose value is false, which will result in a simpler API.
#145 CurrentlyPlayback should be renamed CurrentPlayback, since CurrentlyPlayback doesn't make sense.
#157 CurrentlyPlayingContext::timestamp should be a DateTime<Utc>.
#157 CurrentlyPlayingContext::progress_ms should be progress: Duration.
Since CurrentlyPlaybackis a superset of CurrentlyPlaying, it should probably contain it instead of containing all its fields.
#145 Device is missing the field is_private_session: bool.
#145 Device::id and Device::volume_percent can be null in the Spotify API, so they should be an Option.
#145 FullPlayingContext doesn't exist in the Spotify API. It should be replaced with CurrentlyPlaybackContext.
#145 SimplifiedPlayingContext also doesn't exist, and should be replaced with CurrentlyPlayingContext.
#145 CUDResult is oddly named; it should refer to playlists in its title. PlaylistResult might be a better name. But it's not even necessary at all, the functions can just return String directly.
~~Single-item modules like image should have their items placed in the above scope.~~
#161 Offset should be represented as a Type + id since that is easier for the user.
#157 Offset::position should be a Duration.
~~Page::{limit, offset, total} should be usizes, because they're easier to work with.~~
~~Page::{next, previous} are not useful to the user since they'll use this program's API.~~
~~CursorBasedPage should also use usizes.~~
~~CursorBasedPage should also not include the URLs.~~
#145 Playing should be replaced with CurrentlyPlayingContext, since it is the same.
#185 SimplifiedPlaylist::tracks should be a struct Tracks { total: usize }; this isn't document but is what the API returns.
#145 PlaylistTrack::added_at can be null so should be Option.
#145 PlaylistTrack is poorly named in the Spotify API. It should be called PlaylistItem since it can also contain episodes.
#194 Allow PlaylistTrack/PlaylistItem to contain episodes.
~~RecommendationsSeed::{after_filtering_size, after_relinking_size, initial_pool_size} should be usizes since they are easier to use.~~
~~SearchResult should be a struct not an enum, since you can search for multiple items at once.~~(You have to specify the searchType when using search endpoint, so you can't search for multiple items at once.)
#157 SimplifiedEpisode::duration_ms should be a Duration.
#145 language shouldn't be in SimplifiedEpisode if it's deprecated.
#157 ResumePoint::resume_position_ms should be a Duration.
~~SimplifiedTrack::disc_number should be a usize, for ease of use.~~
#145 SimplfiiedTrack is missing the is_playable, linked_from and restrictions fields.
~~SimplifiedTrack::track_number should be a usize, again for ease of use.~~
#145 PublicUser::images should be a Vec<Image> and represent None as an empty Vec.
#145 PrivateUser is missing the product field, which should be an enum Subscription { Premium, Free }.
~~Country should probably be moved to a separate crate like isocountry~~(It's not that necessary)
~~IncludeExternal can just be replaced with a bool in the search function.~~
#128 Given that ClientError exists I'm not sure on the purpose of Error and ErrorKind; are they necessary?

Internals

#129 The Authorization header is constructed manually instead of using Reqwest's bearer_auth method.
#206 Use iterators wherever possible
#129 More idiomatic Option and Result parsing, avoid so many match expressions
Reduce endpoints, make them as concise as possible:
- #200 Is it possible to use HashMap<&str, &str> instead of HashMap<String, String> to avoid so many to_string and to_owned?
- #202 Maybe a hashmap macro like json?
- ~~Is it possible that endpoint_{get,post,put,delete} methods also run convert_result inside?~~
#189 More tests for methods like get_uri and get_id
#257, #224 Clean up all TODOs
Remove all possible unwrap and panic, or attach an explanation next to them.
#250 Remove pub(in crate)s inside already pub(in crate) modules; only use (in crate) when necessary (reduce their usage as much as possible or it becomes a mess). You can find all instances with rg 'pub\s*\((in crate|crate)\)'
#257 Make sure the visibilities are correct, we don't want to export internal parts of the library to keep the docs clean.
#257 Add a bit of logging to the main crate

Originally created by @Kestrer on GitHub (Sep 13, 2020). Original GitHub issue: https://github.com/ramsayleung/rspotify/issues/127 This is a meta-issue of many issues with rspotify. Some of it's opinionated but most of it I think is just an improvement. ### General - [x] #173 The crate root contains nothing other than modules. This means that paths get very long and verbose (e.g. `rspotify::client::Spotify`). The entire contents of the `client` module should be moved to the crate root to make it easier to access. - [x] #128 `senum`; this module contains all the enums used (and also `Error` for some reason), but could be split into more fitting modules such as `model`. - [x] #129 Rspotify is designed for use from a CLI. Lots of the functions read from stdin, but this is out of scope for this library. Either they should be removed or contained in a `cli`/`tui`/`terminal`/`console` module and feature-gated. - [x] #129 This library extensively uses a fake builder pattern - it's like the real builder pattern but it's all the same type. To add proper type safety the builder patterns should be properly implemented in their own types. - [x] #128 Many functions like `Spotify::tracks` take in a `Vec<&str>`/`Vec<String>`/`&[String]`/`&[&str]`, when they should take in an `impl IntoIterator<Item = &str>`. - [x] #128 Some endpoints and functions in Rspotify take `String` as parameters instead of `&str`. - [x] ~~`limit` and `offset` parameters should universally be `usize`s as they are easier to work with from Rust.~~ - [x] #129 Actual `Default::default impl` (see `TokenInfo::default`) - [x] #129 `Spotify::get_uri` should use`format!`: ```rust fn get_uri(&self, _type: Type, _id: &str) -> String { format!("spotify:{}:{}", _type.as_str(), self.get_id(_type, _id)) } ``` - [x] #129 Fix the documentation links to Spotify endpoints and make the format more consistent - [x] #161 Make a separate type for IDs parsed by `Spotify::get_id` - [x] #226 Authenticated tests should actually not modify the user's account and cause minimal changes (as in, don't completely delete the user's library for a test). ### OAuth - [x] #185 `TokenInfo` stores `expires_in` as a `u32`, and additionally has the `expires_at` field. This is not easy to use, instead it should have a single `expires` field with the type `Instant`. - [x] #185 `TokenInfo` stores a `scope: String`, when it should store a `scope: Vec<String>` obtained by splitting the scopes by spaces. - [x] #129 `TokenInfo` stores `token_type`, which is useless since it is always `Bearer`. - [x] #129 The `SpotifyClientCredentials` and `SpotifyOAuth` have a superfluous `Spotify` prefix; just `ClientCredentials` would be shorter and not duplicate the crate name. - [x] #129 `SpotifyClientCredentials` contains not only the user's client credentials but also the token info if present, so the name is misleading. In fact the `SpotifyOAuth` struct duplicates the `client_id` and `client_secret` fields due to the lack of a real `ClientCredentials` type. - [x] #129 A new `Client` is created every time `fetch_access_token` is called. This is very inefficient. Instead, the authorization logic should be moved to `Spotify` allowing it to reuse the same client. - [x] #129 As far as I can tell `token_info` is never mutated on `SpotifyClientCredentials`. This means that once the initial token has expired `rspotify` will get a new token _every time_. The solution is to wrap `TokenInfo` into a `Mutex` and mutate it when appropriate. - [x] #129 `Spotify` contains an `access_token` _twice_; once in the struct itself and once in `client_credentials_manager.token_info_access_token`. ### Utils - [x] #128 `convert_map_to_string` is better served by `serde_urlencoded` or even better Reqwest's [`query`](https://docs.rs/reqwest/0.10.8/reqwest/struct.RequestBuilder.html#method.query) method. - [x] A few of these functions like `generate_random_string` should be private. - [x] #185 `datetime_to_timestamp` is only necessary due to this library's use of numbers instead of `Instant`s or `Duration`s. - [x] #129 The `token` functions should probably be methods of `Spotify` instead of standalone functions. - [x] #185 If all the above suggestions are incorporated the `utils` module can be removed entirely, and any remaining functions can simply be placed in the crate root for easy access. ### Model - [x] #128 The `model` module is not reexported from the crate root. Having `pub use model::*` and `pub use {album, artist, audio, etc}::*` would keep the docs uncluttered and allow users to write `rspotify::FullAlbum` instead of the overly verbose `rspotify::model::album::FullAlbum`. - [x] #145 Derive `PartialEq` (and `Eq` if possible) for everything. - [x] #244 The `_type` fields on the models are unnecessary data. It would be better to use a macro to create constants that can only serialize to and deserialize as their string, for example `TypeAlbum` which is represented in JSON as only `"album"`. This avoids carrying extra data while still storing the `type` field. - [x] ~~The `uri` and `href` fields are unnecessary, since it can be recreated easily. Make these methods instead.~~ - [x] #145 `copyrights` is represented as a `Vec<HashMap<String, String>>`; a much more accurate representation is `Vec<Copyright>` and ```rust struct Copyright { text: String, r#type: CopyrightType, } enum CopyrightType { P, C, } ``` - [x] #145 `FullAlbum::release_date` and `FullAlbum::release_date_precision` are `String`s. Instead, it should be deserialized into a proper date type like `NaiveDate` and a `enum DatePrecision { Year, Month, Day }`. - [x] #145 The same applies to `SimplifiedEpisode` and `FullEpisode`. - [x] #157 Types like `FullAlbums`, `PageSimplifiedAlbums`, etc exist as wrappers. This makes the API more complex. Instead, have that type exist solely in the API function that needs it, and then return a `Vec<FullAlbum>`. This drastically reduces the number of public API types. - [x] #145 `Restrictions` is located in the `album` module, despite it not having any particular relevance to albums. - [x] #145 `{FullArtist, FullPlaylist}::followers` is a `HashMap<String, Option<Value>>`. It should be a `struct Followers { total: usize }`. - [x] #177 `AudioAnalysisSection::mode` and `AudioFeatures::mode` are `f32`s but should be `Option<Mode>`s where `enum Mode { Major, Minor }` as it is more useful. - [x] #145 This is more opinionated but `AudioAnalysisSection` and `AudioAnalysisSegment` could contain a `#[serde::flatten]`ed `TimeInterval` instead of the having the fields inline. - [x] #145 `AudioAnalysisMeasure` is identical to `TimeInterval`, and so should be replaced by it. - [x] ~~`AudioFeatures::analysis_url` and `AudioFeatures::track_href` aren't useful for the user, since they will access it via this library.~~(The assumption about how do users use this library is not accurate, since we have no idea about how do users use this library) - [x] #157 `AudioFeatures::duration_ms` should be replaced with a `duration` of type `Duration`. - ~~[ ] `Context` contains a `uri`, but since those aren't particularly useful it should be replaced with a transformer into an `id`.~~ - [x] #145 `Actions::disallows` can be replaced with a `Vec<DisallowKey>` or `HashSet<DisallowKey>` by removing all entires whose value is `false`, which will result in a simpler API. - [x] #145 `CurrentlyPlayback` should be renamed `CurrentPlayback`, since `CurrentlyPlayback` doesn't make sense. - [x] #157 `CurrentlyPlayingContext::timestamp` should be a `DateTime<Utc>`. - [x] #157 `CurrentlyPlayingContext::progress_ms` should be `progress: Duration`. - [x] Since `CurrentlyPlayback`is a superset of `CurrentlyPlaying`, it should probably contain it instead of containing all its fields. - [x] #145 `Device` is missing the field `is_private_session: bool`. - [x] #145 `Device::id` and `Device::volume_percent` can be `null` in the Spotify API, so they should be an `Option`. - [x] #145 `FullPlayingContext` doesn't exist in the Spotify API. It should be replaced with `CurrentlyPlaybackContext`. - [x] #145 `SimplifiedPlayingContext` also doesn't exist, and should be replaced with `CurrentlyPlayingContext`. - [x] #145 `CUDResult` is oddly named; it should refer to playlists in its title. `PlaylistResult` might be a better name. But it's not even necessary at all, the functions can just return `String` directly. - [x] ~~Single-item modules like `image` should have their items placed in the above scope.~~ - [x] #161 `Offset` should be represented as a `Type` + `id` since that is easier for the user. - [x] #157 `Offset::position` should be a `Duration`. - [x] ~~`Page::{limit, offset, total}` should be `usize`s, because they're easier to work with.~~ - [x] ~~`Page::{next, previous}` are not useful to the user since they'll use this program's API.~~ - [x] ~~`CursorBasedPage` should also use `usize`s.~~ - [x] ~~`CursorBasedPage` should also not include the URLs.~~ - [x] #145 `Playing` should be replaced with `CurrentlyPlayingContext`, since it is the same. - [x] #185 `SimplifiedPlaylist::tracks` should be a `struct Tracks { total: usize }`; this isn't document but is what the API returns. - [x] #145 `PlaylistTrack::added_at` can be `null` so should be `Option`. - [x] #145 `PlaylistTrack` is poorly named in the Spotify API. It should be called `PlaylistItem` since it can also contain episodes. - [x] #194 Allow `PlaylistTrack`/`PlaylistItem` to contain episodes. - [x] ~~`RecommendationsSeed::{after_filtering_size, after_relinking_size, initial_pool_size}` should be `usize`s since they are easier to use.~~ - [x] ~~`SearchResult` should be a struct not an enum, since you can search for multiple items at once.~~(You have to specify the `searchType` when using `search` endpoint, so you can't search for multiple items at once.) - [x] #157 `SimplifiedEpisode::duration_ms` should be a `Duration`. - [x] #145 `language` shouldn't be in `SimplifiedEpisode` if it's deprecated. - [x] #157 `ResumePoint::resume_position_ms` should be a `Duration`. - [x] ~~`SimplifiedTrack::disc_number` should be a `usize`, for ease of use.~~ - [x] #145 `SimplfiiedTrack` is missing the `is_playable`, `linked_from` and `restrictions` fields. - [x] ~~`SimplifiedTrack::track_number` should be a `usize`, again for ease of use.~~ - [x] #145 `PublicUser::images` should be a `Vec<Image>` and represent `None` as an empty `Vec`. - [x] #145 `PrivateUser` is missing the `product` field, which should be an `enum Subscription { Premium, Free }`. - [x] ~~`Country` should probably be moved to a separate crate like [isocountry](https://lib.rs/isocountry)~~(It's not that necessary) - [x] ~~`IncludeExternal` can just be replaced with a `bool` in the search function.~~ - [x] #128 Given that `ClientError` exists I'm not sure on the purpose of `Error` and `ErrorKind`; are they necessary? ### Internals - [x] #129 The Authorization header is constructed manually instead of using Reqwest's [`bearer_auth`](https://docs.rs/reqwest/0.10.8/reqwest/struct.RequestBuilder.html#method.bearer_auth) method. - [x] #206 Use iterators wherever possible - [x] #129 More idiomatic `Option` and `Result` parsing, avoid so many `match` expressions - Reduce endpoints, make them as concise as possible: - [x] #200 Is it possible to use `HashMap<&str, &str>` instead of `HashMap<String, String>` to avoid so many `to_string` and `to_owned`? - [x] #202 Maybe a `hashmap` macro like `json`? - [x] ~~Is it possible that `endpoint_{get,post,put,delete}` methods also run `convert_result` inside?~~ - [x] #189 More tests for methods like `get_uri` and `get_id` - [x] #257, #224 Clean up all TODOs - [x] Remove all possible `unwrap` and `panic`, or attach an explanation next to them. - [x] #250 Remove `pub(in crate)`s inside already `pub(in crate)` modules; only use `(in crate)` when necessary (reduce their usage as much as possible or it becomes a mess). You can find all instances with `rg 'pub\s*\((in crate|crate)\)'` - [x] #257 Make sure the visibilities are correct, we don't want to export internal parts of the library to keep the docs clean. - [x] #257 Add a bit of logging to the main crate

kerem

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

closed this issue
added the
enhancement

help wanted

discussion
labels

kerem commented

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

Author

Owner

@marioortizmanero commented on GitHub (Sep 13, 2020):

Hello @Koxiaet!

Thanks a lot for compiling a list of issues you found with Rpotify. I completely agree with most of them, I'll just write my thoughts below about some of them:

Rspotify is designed for use from a CLI. Lots of the functions read from stdin, but this is out of scope for this library. Either they should be removed or contained in a cli/tui/terminal/console module and feature-gated.

I recently created an issue that removed lots of unnecessary dependencies and attempted to make this library more modular. It introduced a new feature called browser (it was originally going to be cli), which is activated by default and can be used to disable the util::request_token and similar methods for those that don't need CLI functionalities. Here's the related issue: #108 and the PR that implemented it: #110 (it's still in master, unreleased). I figured that these functions were used so much that browser should be enabled by default, but I would love to hear what you think about that.

This library extensively uses a fake builder pattern - it's like the real builder pattern but it's all the same type. To add proper type safety the builder patterns should be properly implemented in their own types.

That is something I realized and disliked as well. I opened the #109 issue, which proposes using derive_builder to have a consistent pattern. But I think it doesn't play well with the blocking module when trying to avoid code repetition. Once #120 is done I can figure that out.

TokenInfo stores a scope: String, when it should store a scope: Vec obtained by splitting the scopes by spaces.

I was thinking that we could create a custom scope struct/enum to hold these values instead of strings. But it might be overkill, I'm not really sure.

TokenInfo stores token_type, which is useless since it is always Bearer

I think that's still useful to have around. I'm betting that the Spotify Web API devs put that parameter in there for a reason, otherwise it wouldn't exist at all. It might change in the future, I wouldn't touch that.

The SpotifyClientCredentials and SpotifyOAuth have a superfluous Spotify prefix; just ClientCredentials would be shorter and not duplicate the crate name.

Completely agree. In fact, I would name SpotifyClientCredentials just Credentials. They're obviously for the client.

A new Client is created every time fetch_access_token is called. This is very inefficient. Instead, the authorization logic should be moved to Spotify allowing it to reuse the same client.

The SpotifyClientCredentials/SpotifyOAuth structs are confusing altogether. They share lots of methods. These could be reworked to make them more straightforward.

As far as I can tell token_info is never mutated on SpotifyClientCredentials. This means that once the initial token has expired rspotify will get a new token every time. The solution is to wrap TokenInfo into a Mutex and mutate it when appropriate.

I'm not sure I fully understand that. We were thinking of creating an automatically refreshing token at #4, if that's what you're talking about. But it's not very clear how it can be implemented yet.

Given that ClientError exists I'm not sure on the purpose of Error and ErrorKind; are they necessary?

I hadn't even seen these yet lol. I think they can be removed without a problem.

I also wanted to suggest more things, like using iterators where possible and trying to make the endpoint implementations as concise as possible. May I edit your post to add these smaller improvements?

Respect to how this could be implemented:

I would group up these issues and release them split in different smaller versions, instead of releasing a single one with lots of breaking changes. That way it might be easier to migrate to newer versions.
Before working on any of this, #120 should be worked on first to avoid having to write the code for both the async and blocking versions of the library. It's important to make sure that the new blocking implementation works well with your proposed changes before it's merged to avoid running into issues later.
Are you planning on writing any code / creating PRs? If that is the case, let us know so that you can become a contributor.

@marioortizmanero commented on GitHub (Sep 13, 2020): Hello @Koxiaet! Thanks a lot for compiling a list of issues you found with Rpotify. I completely agree with most of them, I'll just write my thoughts below about some of them: > Rspotify is designed for use from a CLI. Lots of the functions read from stdin, but this is out of scope for this library. Either they should be removed or contained in a cli/tui/terminal/console module and feature-gated. I recently created an issue that removed lots of unnecessary dependencies and attempted to make this library more modular. It introduced a new feature called `browser` (it was originally going to be `cli`), which is activated by default and can be used to disable the `util::request_token` and similar methods for those that don't need CLI functionalities. Here's the related issue: #108 and the PR that implemented it: #110 (it's still in master, unreleased). I figured that these functions were used so much that `browser` should be enabled by default, but I would love to hear what you think about that. > This library extensively uses a fake builder pattern - it's like the real builder pattern but it's all the same type. To add proper type safety the builder patterns should be properly implemented in their own types. That is something I realized and disliked as well. I opened the #109 issue, which proposes using `derive_builder` to have a consistent pattern. But I think it doesn't play well with the blocking module when trying to avoid code repetition. Once #120 is done I can figure that out. > TokenInfo stores a scope: String, when it should store a scope: Vec<String> obtained by splitting the scopes by spaces. I was thinking that we could create a custom `scope` struct/enum to hold these values instead of strings. But it might be overkill, I'm not really sure. > TokenInfo stores token_type, which is useless since it is always Bearer I think that's still useful to have around. I'm betting that the Spotify Web API devs put that parameter in there for a reason, otherwise it wouldn't exist at all. It might change in the future, I wouldn't touch that. > The SpotifyClientCredentials and SpotifyOAuth have a superfluous Spotify prefix; just ClientCredentials would be shorter and not duplicate the crate name. Completely agree. In fact, I would name `SpotifyClientCredentials` just `Credentials`. They're obviously for the client. > A new Client is created every time fetch_access_token is called. This is very inefficient. Instead, the authorization logic should be moved to Spotify allowing it to reuse the same client. The SpotifyClientCredentials/SpotifyOAuth structs are confusing altogether. They share lots of methods. These could be reworked to make them more straightforward. > As far as I can tell token_info is never mutated on SpotifyClientCredentials. This means that once the initial token has expired rspotify will get a new token every time. The solution is to wrap TokenInfo into a Mutex and mutate it when appropriate. I'm not sure I fully understand that. We were thinking of creating an automatically refreshing token at #4, if that's what you're talking about. But it's not very clear how it can be implemented yet. > Given that ClientError exists I'm not sure on the purpose of Error and ErrorKind; are they necessary? I hadn't even seen these yet lol. I think they can be removed without a problem. --- I also wanted to suggest more things, like using iterators where possible and trying to make the endpoint implementations as concise as possible. May I edit your post to add these smaller improvements? --- Respect to how this could be implemented: * I would group up these issues and release them split in different smaller versions, instead of releasing a single one with lots of breaking changes. That way it might be easier to migrate to newer versions. * Before working on any of this, #120 should be worked on first to avoid having to write the code for both the async and blocking versions of the library. It's important to make sure that the new blocking implementation works well with your proposed changes before it's merged to avoid running into issues later. * Are you planning on writing any code / creating PRs? If that is the case, let us know so that you can become a contributor.

kerem commented

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

Author

Owner

@Kestrer commented on GitHub (Sep 13, 2020):

Thanks for the response!

It introduced a new feature called browser (it was originally going to be cli), which is activated by default and can be used to disable the util::request_token and similar methods for those that don't need CLI functionalities.

That's good. However nothing in util::request_token says "I read from stdin", so especially if browser is activated by default users could very likely accidentally use it in an app that shouldn't read from stdin - this is why having a CLI-dedicated module might be a good idea, or renaming it to something like terminal_request_token.

I think that's still useful to have around. I'm betting that the Spotify Web API devs put that parameter in there for a reason, otherwise it wouldn't exist at all. It might change in the future, I wouldn't touch that.

The Spotify devs put it in there because it's in the OAuth2 spec. If you think there's a possibility of more methods being added then it'd probably be best to have a #[non_exhaustive] enum TokenType { Bearer }.

As far as I can tell token_info is never mutated on SpotifyClientCredentials. This means that once the initial token has expired rspotify will get a new token every time. The solution is to wrap TokenInfo into a Mutex and mutate it when appropriate.

I'm not sure I fully understand that.

On line 165 of oauth2.rs, if there is no token because it's expired a new token is requested from Spotify, and this function is called on every request, so after the initial token has expired every request becomes two requests. #4 does seem to be very similar to this.

May I edit your post to add these smaller improvements?

Of course!

I would group up these issues and release them split in different smaller versions, instead of releasing a single one with lots of breaking changes. That way it might be easier to migrate to newer versions.

Personally I would rather have to do one large migration than have to repeatedly update rspotify every so often. Users probably won't update dependencies that often anyway so if they're released close together it will have the effect of one release.

Are you planning on writing any code / creating PRs? If that is the case, let us know so that you can become a contributor.

Unfortunately I'm working on other projects at the moment so I can't, sorry.

@Kestrer commented on GitHub (Sep 13, 2020): Thanks for the response! > It introduced a new feature called browser (it was originally going to be cli), which is activated by default and can be used to disable the util::request_token and similar methods for those that don't need CLI functionalities. That's good. However nothing in `util::request_token` says "I read from stdin", so especially if `browser` is activated by default users could very likely accidentally use it in an app that shouldn't read from stdin - this is why having a CLI-dedicated module might be a good idea, or renaming it to something like `terminal_request_token`. > I think that's still useful to have around. I'm betting that the Spotify Web API devs put that parameter in there for a reason, otherwise it wouldn't exist at all. It might change in the future, I wouldn't touch that. The Spotify devs put it in there because [it's in the OAuth2 spec](https://tools.ietf.org/html/rfc6749#section-7.1). If you think there's a possibility of more methods being added then it'd probably be best to have a `#[non_exhaustive] enum TokenType { Bearer }`. > > As far as I can tell token_info is never mutated on SpotifyClientCredentials. This means that once the initial token has expired rspotify will get a new token every time. The solution is to wrap TokenInfo into a Mutex and mutate it when appropriate. > > I'm not sure I fully understand that. [On line 165 of oauth2.rs](https://github.com/ramsayleung/rspotify/blob/master/src/oauth2.rs#L165), if there is no token because it's expired a new token is requested from Spotify, and this function [is called on every request](https://github.com/ramsayleung/rspotify/blob/master/src/client.rs#L156), so after the initial token has expired every request becomes two requests. #4 does seem to be very similar to this. > May I edit your post to add these smaller improvements? Of course! > I would group up these issues and release them split in different smaller versions, instead of releasing a single one with lots of breaking changes. That way it might be easier to migrate to newer versions. Personally I would rather have to do one large migration than have to repeatedly update rspotify every so often. Users probably won't update dependencies that often anyway so if they're released close together it will have the effect of one release. > Are you planning on writing any code / creating PRs? If that is the case, let us know so that you can become a contributor. Unfortunately I'm working on other projects at the moment so I can't, sorry.

kerem commented

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

Author

Owner

@marioortizmanero commented on GitHub (Sep 14, 2020):

That's good. However nothing in util::request_token says "I read from stdin", so especially if browser is activated by default users could very likely accidentally use it in an app that shouldn't read from stdin - this is why having a CLI-dedicated module might be a good idea, or renaming it to something like terminal_request_token.

Yes, the documentation could definitely be improved. I'll take a look at what other functions use stdin and add them to a cli feature.

The Spotify devs put it in there because it's in the OAuth2 spec. If you think there's a possibility of more methods being added then it'd probably be best to have a #[non_exhaustive] enum TokenType { Bearer }.

Ah, thanks for the link. It seems that the token type changes quite a bit how it works, so if they ever changed it we'd have to include some breaking changes anyway. It might be better to just remove it for now.

Personally I would rather have to do one large migration than have to repeatedly update rspotify every so often. Users probably won't update dependencies that often anyway so if they're released close together it will have the effect of one release.

If I upgraded some library and saw a list of 80+ changes I'd be a bit thrown off... But yeah it might be easier to upgrade at once than mutiple times.

@marioortizmanero commented on GitHub (Sep 14, 2020): > That's good. However nothing in util::request_token says "I read from stdin", so especially if browser is activated by default users could very likely accidentally use it in an app that shouldn't read from stdin - this is why having a CLI-dedicated module might be a good idea, or renaming it to something like terminal_request_token. Yes, the documentation could definitely be improved. I'll take a look at what other functions use stdin and add them to a `cli` feature. > The Spotify devs put it in there because it's in the OAuth2 spec. If you think there's a possibility of more methods being added then it'd probably be best to have a #[non_exhaustive] enum TokenType { Bearer }. Ah, thanks for the link. It seems that the token type changes quite a bit how it works, so if they ever changed it we'd have to include some breaking changes anyway. It might be better to just remove it for now. > Personally I would rather have to do one large migration than have to repeatedly update rspotify every so often. Users probably won't update dependencies that often anyway so if they're released close together it will have the effect of one release. If I upgraded some library and saw a list of 80+ changes I'd be a bit thrown off... But yeah it might be easier to upgrade at once than mutiple times.

kerem commented

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

Author

Owner

@ramsayleung commented on GitHub (Sep 18, 2020):

Hi @Koxiaet, thanks for your suggestion and "complaint", it does point a lot of things out.　Let's slow down and take our times to discuss them all one by one.

senum; this module contains all the enums used (and also Error for some reason), but could be split into more fitting modules such as model

Yes, the single file senum is a little bit clumsy, I would be a good idea to split it up.

limit and offset parameters should universally be usizes as they are easier to work with from Rust.

As for the original thought of limit and offset parameters, I have explained it in this issue: https://github.com/ramsayleung/rspotify/pull/120#issuecomment-683361179

convert_map_to_string is better served by serde_urlencoded or even better Reqwest's query method.

It's better to reused the mature library instead of reinventing wheel.

A few of these functions like generate_random_string should be private.

No really, generate_random_string and datetime_to_timestamp are used by blocking module and async module, which should not be private. And it's unnecessary to maintain two set of utility functions, I have removed some of them as much as possible.

TokenInfo stores expires_in as a u32, and additionally has the expires_at field. This is not easy to use, instead it should have a single expires field with the type Instant.

use Instant is a great idea, but the original reason of using expires_at and expires_in is serde doesn't support to deserialize/serialize Instant natively.

TokenInfo stores token_type, which is useless since it is always Bearer

yes, it is always Bearer for now, it's a field designed to support other type in the feature. If it's Bearer forever, Spotify API doesn't need this field: https://developer.spotify.com/documentation/general/guides/authorization-guide/

Page::{limit, offset, total} should be usizes, because they're easier to work with.

well, limit, offset, total are all u32 now, and the difference between u32 and usize is that u32 is fixed bytes length (width), usize depends on architecture (x64 or x86), and limit, offset, total have nothing to do with the size of address space, so I prefer to keep them as u32

AudioFeatures::duration_ms should be replaced with a duration of type Duration.
Offset::position should be a Duration
....

You have pointed a lot things out about Duration, the reason why I preferred u32 to Duration is that u32 is more friendly with serde. The duration field returned from most of endpoints is integer, it's not that easy to deserialize to a Duration struct.

allow users to write rspotify::FullAlbum instead of the overly verbose rspotify::model::album::FullAlbum.

It's a good idea, but I prefer to keep model module name like rspotify::model::FullAlbum

Derive PartialEq (and Eq if possible) for everything.

What's the point of this suggestion?

Fix the documentation links to Spotify endpoints and make the format more consistent

Is there any link broken and inconsistent documentation? Would you like to point it out.

@ramsayleung commented on GitHub (Sep 18, 2020): Hi @Koxiaet, thanks for your suggestion and "complaint", it does point a lot of things out.　Let's slow down and take our times to discuss them all one by one. > senum; this module contains all the enums used (and also Error for some reason), but could be split into more fitting modules such as model Yes, the single file `senum` is a little bit clumsy, I would be a good idea to split it up. > limit and offset parameters should universally be usizes as they are easier to work with from Rust. As for the original thought of `limit` and `offset` parameters, I have explained it in this issue: https://github.com/ramsayleung/rspotify/pull/120#issuecomment-683361179 > convert_map_to_string is better served by serde_urlencoded or even better Reqwest's query method. It's better to reused the mature library instead of reinventing wheel. > A few of these functions like generate_random_string should be private. No really, `generate_random_string` and `datetime_to_timestamp` are used by `blocking` module and `async` module, which should not be private. And it's unnecessary to maintain two set of utility functions, I have removed some of them as much as possible. > TokenInfo stores expires_in as a u32, and additionally has the expires_at field. This is not easy to use, instead it should have a single expires field with the type Instant. use Instant is a great idea, but the original reason of using `expires_at` and `expires_in` is `serde` doesn't support to deserialize/serialize `Instant` natively. > TokenInfo stores token_type, which is useless since it is always Bearer yes, it is always `Bearer` **for now**, it's a field designed to support other type in the feature. If it's `Bearer` forever, Spotify API doesn't need this field: https://developer.spotify.com/documentation/general/guides/authorization-guide/ > Page::{limit, offset, total} should be usizes, because they're easier to work with. well, `limit`, `offset`, `total` are all `u32` now, and the difference between `u32` and `usize` is that `u32` is fixed bytes length (width), `usize` depends on architecture (x64 or x86), and `limit`, `offset`, `total` have nothing to do with the size of address space, so I prefer to keep them as `u32` > AudioFeatures::duration_ms should be replaced with a duration of type Duration. > Offset::position should be a Duration > .... You have pointed a lot things out about `Duration`, the reason why I preferred `u32` to `Duration` is that `u32` is more friendly with `serde`. The `duration` field returned from most of endpoints is integer, it's not that easy to `deserialize` to a `Duration` struct. > allow users to write `rspotify::FullAlbum` instead of the overly verbose `rspotify::model::album::FullAlbum`. It's a good idea, but I prefer to keep `model` module name like `rspotify::model::FullAlbum` > Derive PartialEq (and Eq if possible) for everything. What's the point of this suggestion? > Fix the documentation links to Spotify endpoints and make the format more consistent Is there any link broken and inconsistent documentation? Would you like to point it out.

kerem commented

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

Author

Owner

@marioortizmanero commented on GitHub (Sep 18, 2020):

As for the original thought of limit and offset parameters, I have explained it in this issue: #120 (comment)

I've actually spent a lot of time trying to figure out how optional parameters can be implemented for APIs like Rspotify. I was even writing a small blog post with all the options, but I haven't finished it yet. I'll share it when I'm done.

@marioortizmanero commented on GitHub (Sep 18, 2020): > As for the original thought of limit and offset parameters, I have explained it in this issue: #120 (comment) I've actually spent a lot of time trying to figure out how optional parameters can be implemented for APIs like Rspotify. I was even writing a small blog post with all the options, but I haven't finished it yet. I'll share it when I'm done.

kerem commented

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

Author

Owner

@Kestrer commented on GitHub (Sep 23, 2020):

well, limit, offset, total are all u32 now, and the difference between u32 and usize is that u32 is fixed bytes length (width), usize depends on architecture (x64 or x86), and limit, offset, total have nothing to do with the size of address space, so I prefer to keep them as u32

It's true they don't, but since Spotify doesn't specify a limit we might as well choose the integer type that is the easiest to use.

It's a good idea, but I prefer to keep model module name like rspotify::model::FullAlbum

It's already supported now anyway, but we shouldn't force users to write out the verbose path especially if a less verbose but unambiguous path is possible. It's just a reexport so you'll still be able to write it out in full if you wish, but people can choose which they prefer.

Derive PartialEq (and Eq if possible) for everything.

What's the point of this suggestion?

Someone might want to check if two tracks are equal. In fact this could even just check the equality of the IDs for efficiency.

Is there any link broken and inconsistent documentation? Would you like to point it out.

Track link's is broken, and I think there were a couple others too? It's probably best to just search for ]h, ] h etc in all the source code.

@Kestrer commented on GitHub (Sep 23, 2020): > well, limit, offset, total are all u32 now, and the difference between u32 and usize is that u32 is fixed bytes length (width), usize depends on architecture (x64 or x86), and limit, offset, total have nothing to do with the size of address space, so I prefer to keep them as u32 It's true they don't, but since Spotify doesn't specify a limit we might as well choose the integer type that is the easiest to use. > It's a good idea, but I prefer to keep model module name like rspotify::model::FullAlbum It's already supported now anyway, but we shouldn't force users to write out the verbose path especially if a less verbose but unambiguous path is possible. It's just a reexport so you'll still be able to write it out in full if you wish, but people can choose which they prefer. > > Derive PartialEq (and Eq if possible) for everything. > > What's the point of this suggestion? Someone might want to check if two tracks are equal. In fact this could even just check the equality of the IDs for efficiency. > Is there any link broken and inconsistent documentation? Would you like to point it out. [Track link's](https://docs.rs/rspotify/0.10.0/rspotify/model/track/struct.TrackLink.html) is broken, and I think there were a couple others too? It's probably best to just search for `]h`, `] h` etc in all the source code.

kerem commented

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

Author

Owner

@ramsayleung commented on GitHub (Oct 24, 2020):

It's true they don't, but since Spotify doesn't specify a limit we might as well choose the integer type that is the easiest to use.

I am a bit unsure about the conclusion that we might as well choose the integer type that is the easiest to use. why would you think usize is the easiest to use? Sorry for not getting your point.
Check this post for more details, I32 vs isize, u32 vs usize, I agreed with cuviper's point:

Use u32 and i32 when you just want numbers.

FullAlbum::release_date and FullAlbum::release_date_precision are Strings. Instead, it should be deserialized into a proper date type like NaiveDate and a enum DatePrecision { Year, Month, Day }.

I agreed with the later part, release_date_precision should be a enum DatePrecision { Year, Month, Day }. , but as for release_date, it's not that easy to use a date type like NaiveDate which could be "1981-12-15". "1981", "1981-12", depending on the precision, but NaiveDate can't have a empty month and empty day.

IncludeExternal can just be replaced with a bool in the search function.

I don't think it's necessary, probably Spotify will add some new values for include_external, then bool will not be a good way to go.

@ramsayleung commented on GitHub (Oct 24, 2020): > It's true they don't, but since Spotify doesn't specify a limit we might as well choose the integer type that is the easiest to use. I am a bit unsure about the conclusion that we might as well choose the integer type that is the easiest to use. why would you think `usize` is the easiest to use? Sorry for not getting your point. Check this post for more details, [I32 vs isize, u32 vs usize](https://users.rust-lang.org/t/i32-vs-isize-u32-vs-usize/22657), I agreed with cuviper's point: > Use u32 and i32 when you just want numbers. > FullAlbum::release_date and FullAlbum::release_date_precision are Strings. Instead, it should be deserialized into a proper date type like NaiveDate and a enum DatePrecision { Year, Month, Day }. I agreed with the later part, `release_date_precision` should be a `enum DatePrecision { Year, Month, Day }. `, but as for `release_date`, it's not that easy to use a date type like `NaiveDate` which could be "1981-12-15". "1981", "1981-12", depending on the precision, but `NaiveDate` can't have a empty month and empty day. > `IncludeExternal` can just be replaced with a `bool` in the search function. I don't think it's necessary, probably Spotify will add some new values for `include_external`, then `bool` will not be a good way to go.

kerem commented

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

Author

Owner

@marioortizmanero commented on GitHub (Feb 25, 2021):

Regarding this:

The _type fields on the models are unnecessary data. It would be better to use a macro to create constants that can only serialize to and deserialize as their string, for example TypeAlbum which is represented in JSON as only "album". This avoids carrying extra data while still storing the type field.

We could also do something similar to what's being implemented at #161, which I really like. Just make the thing generic over the type and implement a get_type() method or something like that to access the type if necessary. It might be a better idea because it doesn't need macros, the Album variants are all grouped up in the same struct instead of multiple ones, and it's type safe.

Loving how we're progressing on this btw!! Not that much work to close it :)

@marioortizmanero commented on GitHub (Feb 25, 2021): Regarding this: > The `_type` fields on the models are unnecessary data. It would be better to use a macro to create constants that can only serialize to and deserialize as their string, for example `TypeAlbum` which is represented in JSON as only `"album"`. This avoids carrying extra data while still storing the `type` field. We could also do something similar to what's being implemented at #161, which I really like. Just make the thing generic over the type and implement a `get_type()` method or something like that to access the type if necessary. It might be a better idea because it doesn't need macros, the `Album` variants are all grouped up in the same struct instead of multiple ones, and it's type safe. Loving how we're progressing on this btw!! Not that much work to close it :)

kerem commented

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

Author

Owner

@kstep commented on GitHub (Mar 9, 2021):

I think some points can be marked complete now:

Make a separate type for IDs parsed by Spotify::get_id
Offset should be represented as a Type + id since that is easier for the user.

@kstep commented on GitHub (Mar 9, 2021): I think some points can be marked complete now: - Make a separate type for IDs parsed by Spotify::get_id - Offset should be represented as a Type + id since that is easier for the user.

kerem commented

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

Author

Owner

@marioortizmanero commented on GitHub (Mar 9, 2021):

Sure, updated them

@marioortizmanero commented on GitHub (Mar 9, 2021): Sure, updated them

kerem commented

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

Author

Owner

@marioortizmanero commented on GitHub (Apr 20, 2021):

Didn't mean to close this, whoops

@marioortizmanero commented on GitHub (Apr 20, 2021): Didn't mean to close this, whoops

kerem commented

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

Author

Owner

@ramsayleung commented on GitHub (Apr 20, 2021):

Is it possible that endpoint_{get,post,put,delete} methods also run convert_result inside?

As for this TODO item, I have tried, the problem I have occured was that not all endpoints need to return a struct implemented Deserialize trait, which means it doesn't need to call convert_result function. If we run convert_result inside endpoint_{get,post,put,delete} methods, We have to distinguish them between Deserialized structure and something like (). The below code doesn't call convert_result function:

    #[maybe_async]
    pub async fn playlist_replace_tracks<'a>(
        &self,
        playlist_id: &PlaylistId,
        track_ids: impl IntoIterator<Item = &'a TrackId>,
    ) -> ClientResult<()> {
        let uris = track_ids.into_iter().map(|id| id.uri()).collect::<Vec<_>>();

        let params = json!({ "uris": uris });
        let url = format!("playlists/{}/tracks", playlist_id.id());
        self.endpoint_put(&url, &params).await?;

        Ok(())
    }

@ramsayleung commented on GitHub (Apr 20, 2021): > Is it possible that endpoint_{get,post,put,delete} methods also run convert_result inside? As for this TODO item, I have tried, the problem I have occured was that not all endpoints need to return a struct implemented `Deserialize` trait, which means it doesn't need to call `convert_result` function. If we run `convert_result` inside `endpoint_{get,post,put,delete}` methods, We have to distinguish them between `Deserialized structure` and something like `()`. The below code doesn't call `convert_result` function: ```rust #[maybe_async] pub async fn playlist_replace_tracks<'a>( &self, playlist_id: &PlaylistId, track_ids: impl IntoIterator<Item = &'a TrackId>, ) -> ClientResult<()> { let uris = track_ids.into_iter().map(|id| id.uri()).collect::<Vec<_>>(); let params = json!({ "uris": uris }); let url = format!("playlists/{}/tracks", playlist_id.id()); self.endpoint_put(&url, &params).await?; Ok(()) } ```

kerem commented

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

Author

Owner

@marioortizmanero commented on GitHub (Apr 20, 2021):

Ok thanks for trying it out! I'll remove it from the list. It's not really that important and not worth dealing with, then.

@marioortizmanero commented on GitHub (Apr 20, 2021): Ok thanks for trying it out! I'll remove it from the list. It's not really that important and not worth dealing with, then.

kerem commented

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

Author

Owner

@ramsayleung commented on GitHub (Apr 23, 2021):

Use iterators wherever possible

As for this TODO Item, we have changed the functions' signature from Vec<String>/&[String] to impl IntoIterator<Item = &'a str>, except for the fields which contains Vec<T> in src/model module, is there anything else we need to change to Iterator ?

Remove all possible unwrap and panic, or attach an explanation next to them.

There is no panic statement left in this library, as for the unwrap statement, there are two usages:

/// Generate `length` random chars
pub(in crate) fn generate_random_string(length: usize) -> String {
    let alphanum: &[u8] =
        "abcdefghijklmnopqrstuvwxyzABCDEFGHIJKLMNOPQRSTUVWXYZ0123456789".as_bytes();
    let mut buf = vec![0u8; length];
    getrandom(&mut buf).unwrap(); //! unwrap statement
    let range = alphanum.len();

    buf.iter()
        .map(|byte| alphanum[*byte as usize % range] as char)
        .collect()
}

    async fn request<D>(
        &self,
        method: Method,
        url: &str,
        headers: Option<&Headers>,
        add_data: D,
    ) -> ClientResult<String>
    where
        D: Fn(RequestBuilder) -> RequestBuilder,
    {
        let mut request = self.client.request(method.clone(), url);

        // Setting the headers, if any
        if let Some(headers) = headers {
            // The headers need to be converted into a `reqwest::HeaderMap`,
            // which won't fail as long as its contents are ASCII. This is an
            // internal function, so the condition cannot be broken by the user
            // and will always be true.
            //
            // The content-type header will be set automatically.
            let headers = headers.try_into().unwrap(); //! unwrap statement

            request = request.headers(headers);
        }
}

I am thinking about is it necessary to remove these two unwrap statements, what do you guys think?

@ramsayleung commented on GitHub (Apr 23, 2021): > Use iterators wherever possible As for this TODO Item, we have changed the functions' signature from `Vec<String>/&[String]` to `impl IntoIterator<Item = &'a str>`, except for the fields which contains `Vec<T>` in `src/model` module, is there anything else we need to change to `Iterator` ? > Remove all possible `unwrap` and `panic`, or attach an explanation next to them. There is no `panic` statement left in this library, as for the `unwrap` statement, there are two usages: ```rust /// Generate `length` random chars pub(in crate) fn generate_random_string(length: usize) -> String { let alphanum: &[u8] = "abcdefghijklmnopqrstuvwxyzABCDEFGHIJKLMNOPQRSTUVWXYZ0123456789".as_bytes(); let mut buf = vec![0u8; length]; getrandom(&mut buf).unwrap(); //! unwrap statement let range = alphanum.len(); buf.iter() .map(|byte| alphanum[*byte as usize % range] as char) .collect() } async fn request<D>( &self, method: Method, url: &str, headers: Option<&Headers>, add_data: D, ) -> ClientResult<String> where D: Fn(RequestBuilder) -> RequestBuilder, { let mut request = self.client.request(method.clone(), url); // Setting the headers, if any if let Some(headers) = headers { // The headers need to be converted into a `reqwest::HeaderMap`, // which won't fail as long as its contents are ASCII. This is an // internal function, so the condition cannot be broken by the user // and will always be true. // // The content-type header will be set automatically. let headers = headers.try_into().unwrap(); //! unwrap statement request = request.headers(headers); } } ``` I am thinking about is it necessary to remove these two `unwrap` statements, what do you guys think?

kerem commented

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

Author

Owner

@marioortizmanero commented on GitHub (Apr 23, 2021):

As for this TODO Item, we have changed the functions' signature from Vec/&[String] to impl IntoIterator<Item = &'a str>, except for the fields which contains Vec in src/model module, is there anything else we need to change to Iterator ?

https://github.com/ramsayleung/rspotify/blob/master/src/client.rs#L878
maybe these? https://github.com/ramsayleung/rspotify/blob/master/src/client.rs#L1627 (only if it doesn't end up looking worse than right now)

and nothing else afaik.

I am thinking about is it necessary to remove these two unwrap statements, what do you guys think?

I think it's safe to assume that we can use unwrap() in the first one, because even the rand crate does.

And the second does have an explanation, so no issue with that.

I thought we had more unwraps, that's nice :)

@marioortizmanero commented on GitHub (Apr 23, 2021): > As for this TODO Item, we have changed the functions' signature from Vec<String>/&[String] to impl IntoIterator<Item = &'a str>, except for the fields which contains Vec<T> in src/model module, is there anything else we need to change to Iterator ? * https://github.com/ramsayleung/rspotify/blob/master/src/client.rs#L878 * maybe these? https://github.com/ramsayleung/rspotify/blob/master/src/client.rs#L1627 (only if it doesn't end up looking worse than right now) and nothing else afaik. > I am thinking about is it necessary to remove these two unwrap statements, what do you guys think? I think it's safe to assume that we can use `unwrap()` in the first one, because even the `rand` crate does. And the second does have an explanation, so no issue with that. I thought we had more `unwrap`s, that's nice :)

kerem commented

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

Author

Owner

@ramsayleung commented on GitHub (Apr 24, 2021):

https://github.com/ramsayleung/rspotify/blob/master/src/client.rs#L878

I have fixed it in this branch: https://github.com/ramsayleung/rspotify/blob/ramsay_pass_parameters_by_reference/src/client.rs#L862, and I will create a PR after #202 merged

maybe these? https://github.com/ramsayleung/rspotify/blob/master/src/client.rs#L1627 (only if it doesn't end up looking worse than right now)

And I am working on it now. But I am not sure if it's worth replacing with iterator:


        // This map is used to store the intermediate data which lives long enough
        // to be borrowed into the `param`
        let mut map_to_hold_owned_value: HashMap<String, String> = HashMap::new();
        attributes
            .iter()
            // create cartesian product for attributes and prefixes
            .flat_map(|attribute| {
                prefixes
                    .iter()
                    .clone()
                    .map(move |prefix| format!("{}_{}", prefix, attribute))
            })
            .for_each(|param| {
                if let Some(value) = payload.get(&param) {
                    // TODO: not sure if this `to_string` is what we want. It
                    // might add quotes to the strings.
                    map_to_hold_owned_value.insert(param, value.to_string());
                }
            });

        for (ref key, ref value) in &map_to_hold_owned_value {
            params.insert(key, value);
        }

I thought we had more unwraps, that's nice :)

It's nice to have more unwraps :) ?

@ramsayleung commented on GitHub (Apr 24, 2021): > https://github.com/ramsayleung/rspotify/blob/master/src/client.rs#L878 I have fixed it in this branch: https://github.com/ramsayleung/rspotify/blob/ramsay_pass_parameters_by_reference/src/client.rs#L862, and I will create a PR after #202 merged > maybe these? https://github.com/ramsayleung/rspotify/blob/master/src/client.rs#L1627 (only if it doesn't end up looking worse than right now) And I am working on it now. But I am not sure if it's worth replacing with `iterator`: ```rust // This map is used to store the intermediate data which lives long enough // to be borrowed into the `param` let mut map_to_hold_owned_value: HashMap<String, String> = HashMap::new(); attributes .iter() // create cartesian product for attributes and prefixes .flat_map(|attribute| { prefixes .iter() .clone() .map(move |prefix| format!("{}_{}", prefix, attribute)) }) .for_each(|param| { if let Some(value) = payload.get(&param) { // TODO: not sure if this `to_string` is what we want. It // might add quotes to the strings. map_to_hold_owned_value.insert(param, value.to_string()); } }); for (ref key, ref value) in &map_to_hold_owned_value { params.insert(key, value); } ``` > I thought we had more unwraps, that's nice :) It's nice to have more `unwraps` :) ?

kerem commented

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

Author

Owner

@marioortizmanero commented on GitHub (Apr 24, 2021):

And I am working on it now. But I am not sure if it's worth replacing with iterator:

You could've done a collect into HashMap<String, String> instead of using a for_each. Also, does the flat_map really need .clone()? can't you just remove move from the closure?

It's nice to have more unwraps :) ?

No, the opposite, it's nice not to have them :)

@marioortizmanero commented on GitHub (Apr 24, 2021): > And I am working on it now. But I am not sure if it's worth replacing with iterator: You could've done a `collect` into `HashMap<String, String>` instead of using a `for_each`. Also, does the `flat_map` really need `.clone()`? can't you just remove `move` from the closure? > It's nice to have more unwraps :) ? No, the opposite, it's nice not to have them :)

kerem commented

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

Author

Owner

@marioortizmanero commented on GitHub (Jul 8, 2021):

Now that the auth restructure is done, I will focus on, for now:

If anyone else wants to work on these issues do let me know and we can do so together or I'll just do something else (I don't really mind what to do). I will leave on holidays the second half of July so I won't be able to do much until August. But I expect that we might be able to release v0.11 in late-august or september if everything goes well :)

@marioortizmanero commented on GitHub (Jul 8, 2021): Now that the auth restructure is done, I will focus on, for now: * #226 * https://github.com/ramsayleung/rspotify/issues/221 * https://github.com/ramsayleung/rspotify/issues/4 If anyone else wants to work on these issues do let me know and we can do so together or I'll just do something else (I don't really mind what to do). I will leave on holidays the second half of July so I won't be able to do much until August. But I expect that we might be able to release v0.11 in late-august or september if everything goes well :)

kerem commented

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

Author

Owner

@ramsayleung commented on GitHub (Jul 8, 2021):

Wish you have a great holiday :)

@ramsayleung commented on GitHub (Jul 8, 2021): Wish you have a great holiday :)

kerem commented

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

Author

Owner

@marioortizmanero commented on GitHub (Sep 25, 2021):

About this one:

Context contains a uri, but since those aren't particularly useful it should be replaced with a transformer into an id.

It doesn't really matter now that we have custom ID types. Instead of:

let id = ArtistId::from_id(&context.id);

The user can just use:

let id = ArtistId::from_uri(&context.uri);

And use it the exact same way.

So I would remove that one off the list. Otherwise we have to manually deserialize and serialize the Context struct, which is a pain and not really worth it considering the usage is basically the same, and that it would add an unnecessary overhead when deserializing.

@marioortizmanero commented on GitHub (Sep 25, 2021): About this one: > Context contains a uri, but since those aren't particularly useful it should be replaced with a transformer into an id. It doesn't really matter now that we have custom ID types. Instead of: ```rust let id = ArtistId::from_id(&context.id); ``` The user can just use: ```rust let id = ArtistId::from_uri(&context.uri); ``` And use it the exact same way. So I would remove that one off the list. Otherwise we have to manually deserialize and serialize the `Context` struct, which is a pain and not really worth it considering the usage is basically the same, and that it would add an unnecessary overhead when deserializing.

kerem commented

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

Author

Owner

@marioortizmanero commented on GitHub (Oct 8, 2021):

I can't believe we're closing this finally :D

@marioortizmanero commented on GitHub (Oct 8, 2021): I can't believe we're closing this finally :D

kerem referenced this issue

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

[PR #42] [MERGED] get_token processing can be done in two parts #201

Rows
Columns

[GH-ISSUE #127] Meta-Issue #42

General

OAuth

Utils

Model

Internals