Multiple clients via features

[marioortizmanero]

This is a second attempt for fixing #112. The original idea at #120 was to have a blocking module that would call the async implementations with a blocking runtime in order to avoid code duplication. This turned up to be a bad idea, because it wasn't as easy and flexible as I thought. This new approach uses the maybe-async crate, which makes it relatively easy to use multiple HTTP clients in the same crate with features. Instead of having a blocking module, the user may configure what client is used like so: rspotify = { version = "...", default_features = false, features = "client-ureq" } (client-reqwest is still the default).

Advantages of this approach:

• Much cleaner implementation for the "blocking module"
• More flexible than a switch between sync and async code, since multiple clients for each of these can be configured. For now it's only two. This flexibility also makes rspotify faster and less bloated, since the blocking module now uses ureq instead of reqwest, which is much more minimal.
• Just as fast to compile: you'd think that the original approach would be faster to compile because it doesn't need a macro to work. But in the end the macro is necessary to make development less of a pain, so both solutions use macros. I haven't tested this but I'm sure the difference isn't noticeable.
• The endpoint function signatures aren't limited: the original approach required a macro_rules! macro. These are quick and easy to implement, but supporting generic parameters and other compex stuff can get out of hands pretty quickly. This avoids any workarounds for that.
• The derive_deref dependency isn't necessary.

Disadvantages of this approach:

• Only one client may be used at once
• New dependency: maybe-async. It's just a procedural macro so no performance impact whatsoever. This also requires async_trait.

Required changes and progress

• Async client with reqwest
• Blocking client with ureq
• Rewrite OAuth
• Update documentation
• Update tests, make them available for all clients
• Update basic examples
• Update the webapp example
• Open an issue at maybe_async about the parenthesis warnings when using ureq: Parenthesis warnings with synchronous code fMeow/maybe-async-rs#1

Read the CHANGELOG.md additions for more changes (most of them are from #127). I had to rewrite the fetch_access_token logic so that it can use the HTTP client in Spotify, and I ended up improving lots of other stuff I saw. (this is still very WIP, along with the first two points of this section).

I've left lots of TODOs that can be discussed after this is merged. I'd like to finish this ASAP so that other changes can be made without merge conflicts and without having to apply them for the blocking module as well.

Also see the examples, some of them have been modified to show how the API works now.

• Closes Using derive_builder to avoid repetition with the builder pattern #109
• Closes Cleaning up the blocking module #112
• Closes Explicitly read from the environment variables with from_env #116
• Closes Update derive_builder #131

[ramsayleung]

Sorry for missing your comment, I have fixed the webapp example :)

[marioortizmanero]

Ok so I've made a few discussion-related issues recently. The plan is to open some more issues in libraries that use rspotify like spotifyd, spotify-tui or ncspot, and both let them know about the rewrite and ask for their opinion. That should help us to take the decisions. I'll do that once this PR is merged. I won't be making any more commits from now on (other than suggestions you may come up with when reviewing the code).

[ramsayleung]

I have finished the code review, it's a huge PR, takes many time to review, and I have added a few suggestions, you could take time to have a look at them :)

[marioortizmanero]

Sorry what suggestions?

[ramsayleung]

I have left some review comments, does you miss them? And I have merged a PR submitted by other contributor which seems to cause a conflict on file CHANGELOG.md

[marioortizmanero]

I'm afraid I can't find these comments. Can you provide a link? I'll fix the CHANGELOG.md issue.

[ramsayleung]

There are all my review comments, I just forgot to submit my reviews and all comments are pending, sorry for that.

[ramsayleung]

what does D: Fn(&mut Request)->Response mean? I am a little bit confused about that?

[marioortizmanero]

It's just a function to continue building the base Request (with headers and such already) to pass its contents, like the one in src/http/reqwest.rs. You can see its usage later on in this file.

In this case, calling something like send_json with ureq converts the Request into a Response (it's like the call method in Reqwest implicitly), so that's the function signature needed. Using a closure makes it straightforward to choose between GET/POST/PUT/DELETE, and avoids the Content::Json and Content::Form enum I had previously, which was admittedly a mess.

[ramsayleung]

We could have a macro named map! which does the same job as what json! does, to reduce the boilerplate code.

[marioortizmanero]

I did think about it. I initially wanted to use a serde_json::Value type for GET requests (and thus json!) because it makes its usage super easy and clean, but I ended up running into too many issues.

I also tried to make the query type a HashMap<&str, &str> but failed because of ownership issues (this would avoid the to_owned calls). All of the following conditional inserts failed because &str is not owned and it goes out of scope:

        if let Some(market) = market {
            params.insert("market", market.as_str());
        }

But I can give that a try again to see if it's possible to improve before being forced to use a macro (you can give it a try as well). Rather than creating one I'd use existing crates, like maplit.

[marioortizmanero]

I would leave this for a different PR, though.

[ramsayleung]

Is it ready to merge? :)

[marioortizmanero]

I would say so!

[ramsayleung]

Merged :)