Meta-Issue

[Kestrer]

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

• Restructure the authentication process #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.
• Polish Rspotify #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.
• Multiple clients via features #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.
• Multiple clients via features #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.
• Polish Rspotify #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>.
• Polish Rspotify #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.
• Multiple clients via features #129 Actual Default::default impl (see TokenInfo::default)
• Multiple clients via features #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))
    }

• Multiple clients via features #129 Fix the documentation links to Spotify endpoints and make the format more consistent
• Initial id type proposal #161 Make a separate type for IDs parsed by Spotify::get_id
• OAuth tests cleanup, they don't modify the user's profile anymore #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

• Polish Token and 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.
• Polish Token and OAuth #185 TokenInfo stores a scope: String, when it should store a scope: Vec<String> obtained by splitting the scopes by spaces.
• Multiple clients via features #129 TokenInfo stores token_type, which is useless since it is always Bearer.
• Multiple clients via features #129 The SpotifyClientCredentials and SpotifyOAuth have a superfluous Spotify prefix; just ClientCredentials would be shorter and not duplicate the crate name.
• Multiple clients via features #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.
• Multiple clients via features #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.
• Multiple clients via features #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.
• Multiple clients via features #129 Spotify contains an access_token twice; once in the struct itself and once in client_credentials_manager.token_info_access_token.

Utils

• Polish Rspotify #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.
• Polish Token and OAuth #185 datetime_to_timestamp is only necessary due to this library's use of numbers instead of Instants or Durations.
• Multiple clients via features #129 The token functions should probably be methods of Spotify instead of standalone functions.
• Polish Token and OAuth #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

• Polish Rspotify #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.
• Refactor model #145 Derive PartialEq (and Eq if possible) for everything.
• Fix IDs v4 #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.
• Refactor model #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,
}

• Refactor model #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 }.
• Refactor model #145 The same applies to SimplifiedEpisode and FullEpisode.
• Keep polishing the models #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.
• Refactor model #145 Restrictions is located in the album module, despite it not having any particular relevance to albums.
• Refactor model #145 {FullArtist, FullPlaylist}::followers is a HashMap<String, Option<Value>>. It should be a struct Followers { total: usize }.
• Serialize enum as number #177 AudioAnalysisSection::mode and AudioFeatures::mode are f32s but should be Option<Mode>s where enum Mode { Major, Minor } as it is more useful.
• Refactor model #145 This is more opinionated but AudioAnalysisSection and AudioAnalysisSegment could contain a #[serde::flatten]ed TimeInterval instead of the having the fields inline.
• Refactor model #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)
• Keep polishing the models #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.
• Refactor model #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.
• Refactor model #145 CurrentlyPlayback should be renamed CurrentPlayback, since CurrentlyPlayback doesn't make sense.
• Keep polishing the models #157 CurrentlyPlayingContext::timestamp should be a DateTime<Utc>.
• Keep polishing the models #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.
• Refactor model #145 Device is missing the field is_private_session: bool.
• Refactor model #145 Device::id and Device::volume_percent can be null in the Spotify API, so they should be an Option.
• Refactor model #145 FullPlayingContext doesn't exist in the Spotify API. It should be replaced with CurrentlyPlaybackContext.
• Refactor model #145 SimplifiedPlayingContext also doesn't exist, and should be replaced with CurrentlyPlayingContext.
• Refactor model #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.
• Initial id type proposal #161 Offset should be represented as a Type + id since that is easier for the user.
• Keep polishing the models #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.
• Refactor model #145 Playing should be replaced with CurrentlyPlayingContext, since it is the same.
• Polish Token and OAuth #185 SimplifiedPlaylist::tracks should be a struct Tracks { total: usize }; this isn't document but is what the API returns.
• Refactor model #145 PlaylistTrack::added_at can be null so should be Option.
• Refactor model #145 PlaylistTrack is poorly named in the Spotify API. It should be called PlaylistItem since it can also contain episodes.
• Allow PlaylistItem to 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.)
• Keep polishing the models #157 SimplifiedEpisode::duration_ms should be a Duration.
• Refactor model #145 language shouldn't be in SimplifiedEpisode if it's deprecated.
• Keep polishing the models #157 ResumePoint::resume_position_ms should be a Duration.
• SimplifiedTrack::disc_number should be a usize, for ease of use.
• Refactor model #145 SimplfiiedTrack is missing the is_playable, linked_from and restrictions fields.
• SimplifiedTrack::track_number should be a usize, again for ease of use.
• Refactor model #145 PublicUser::images should be a Vec<Image> and represent None as an empty Vec.
• Refactor model #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.
• Polish Rspotify #128 Given that ClientError exists I'm not sure on the purpose of Error and ErrorKind; are they necessary?

Internals

• Multiple clients via features #129 The Authorization header is constructed manually instead of using Reqwest's bearer_auth method.
• Pass parameters by reference and use iterators wherever possible #206 Use iterators wherever possible
• Multiple clients via features #129 More idiomatic Option and Result parsing, avoid so many match expressions
• Reduce endpoints, make them as concise as possible:
  • Replace HashMap<String, String> with HashMap<&str, &str> to reduce allocation #200 Is it possible to use HashMap<&str, &str> instead of HashMap<String, String> to avoid so many to_string and to_owned?
  • Remove Rspotify default parameters and add parameter macros #202 Maybe a hashmap macro like json?
  • Is it possible that endpoint_{get,post,put,delete} methods also run convert_result inside?
• Add unit tests for pure methods #189 More tests for methods like get_uri and get_id
• Fix the remaining TODOs #257, Support re-authenticate automatically and refresh token when it expired. #224 Clean up all TODOs
• Remove all possible unwrap and panic, or attach an explanation next to them.
• Cleanup #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)\)'
• Fix the remaining TODOs #257 Make sure the visibilities are correct, we don't want to export internal parts of the library to keep the docs clean.
• Fix the remaining TODOs #257 Add a bit of logging to the main crate

[marioortizmanero]

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, Blocking module cleanup #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.

[Kestrer]

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.

[marioortizmanero]

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.

[ramsayleung]

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: #120 (comment)

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.

[marioortizmanero]

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.

[Kestrer]

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.

[ramsayleung]

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.

[marioortizmanero]

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 :)

[kstep]

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.

[marioortizmanero]

Sure, updated them

[marioortizmanero]

Didn't mean to close this, whoops

[ramsayleung]

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(())
    }

[marioortizmanero]

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.

[ramsayleung]

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?

[marioortizmanero]

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 :)

[ramsayleung]

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 :) ?

[marioortizmanero]

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]

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

• OAuth tests cleanup, they don't modify the user's profile anymore #226
• Avoiding conflicting features; allow multiple HTTP clients at once #221
• Automatic Re-authentication #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 :)

[ramsayleung]

Wish you have a great holiday :)

[marioortizmanero]

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]

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