Slightly better documentation, comment formatting, proposal for more changes

[marioortizmanero]

Hi @ramsayleung!

I was going to use this library for one of my projects and I've decided I'm going to help a bit. I've formatted consistently and correctly the comments for each endpoint with a vim macro, and have made very small changes to the docs, hoping they're more clear now.

I would love to run a cargo fmt to clean everything up automatically, but I understand it'd be better to first merge the two other PRs pending to avoid messing up the diffs.

Another thing I really want to change is the repetition between blocking/cilent.rs and client.rs. A macro would come in perfectly here to unify blocking and async and I think would make it super easy to add/update new endpoints. I'll make a new PR if you're okay with that (if I manage to get it working). There's got to be a solution for that already anyway, I'll have to investigate. I should open a new issue for that, right?

Edit: I've seen that there's a CI run for cargo fmt, which is failing. Should I just run it in a new commit, then?

[ramsayleung]

I've formatted consistently and correctly the comments for each endpoint with a vim macro, and have made very small changes to the docs, hoping they're more clear now.

It's great that you are helping to polish this crate.

I would love to run a cargo fmt to clean everything up automatically.

So do I, and I have set up the Github Action to run cargo fmt on a CI, perhaps you could this before you make some PRs.

Another thing I really want to change is the repetition between blocking/cilent.rs and client.rs

Sorry for the code smell, it will be great to see only the sole endpoint instead of two almost same repetitions. When I started to refactor this crate to add async/await support, I could not find a proper way to unify blocking and async endpoint currently, finally I chose the clumsy but useful solution, just maintaining two versions of endpoint. It will be awesome if you could find a way to clean it up.

Edit: I've seen that there's a CI run for cargo fmt, which is failing. Should I just run it in a new commit, then?

I think you should fix the errors analyzed by cargo fmt, and create a new commit to trigger the CI.

[marioortizmanero]

Sorry for the code smell, it will be great to see only the sole endpoint instead of two almost same repetitions. When I started to refactor this crate to add async/await support, I could not find a proper way to unify blocking and async endpoint currently, finally I chose the clumsy but useful solution, just maintaining two versions of endpoint. It will be awesome if you could find a way to clean it up.

I will investigate a bit about how this can be done. Another library called tekore does this, so I might take inspiration. In Tekore's case, the library used for the HTTP requests (used to be requests in Tekore) should alternate between blocking and async with a feature that can be toggled for rspotify as well (httpx for Tekore). But that solution seems quite Python-tied, so I might just use a macro. Anyhow, should it stay as always async by default and having blocking requests as an extra feature, or a toggle with either of them chosen with a feature? Because the latter makes sense to me, and would simplify this a bit (?).

I fixed the formatting issues, you can merge now if you want.

[marioortizmanero]

I've tried to remove the repetition in blocking at https://github.com/marioortizmanero/rspotify/tree/blocking-rewrite but I'm honestly not experienced enough with async/tokio yet.

I initially followed what reqwest does for their blocking module, and I wrapped the async implementation in the blocking::Spotify struct, executing its methods synchronously by using a single-threaded runtime. That way, the methods would just be declared like this, which could be implemented in a macro in the main client for ease of use.

I planned on storing said runtime inside the struct itself, but it's quite messy because the runtime is also needed outside of this struct for implementations like SpotifyOAuth and other outer functions. block_on requires a mutable runtime, so it has to be wrapped inside a RefCell to avoid changing the method signatures. This won't work either because it doesn't implement Clone. A global static one would probably be a better idea, but it requires unsafe code to be mutable.

Not sure what to do... Any thoughts? Maybe I should open a new issue for this?

[ramsayleung]

I fixed the formatting issues, you can merge now if you want.

It's my pleasure to do that :)

[ramsayleung]

I initially followed what reqwest does for their blocking module, and I wrapped the async implementation in the blocking::Spotify struct, executing its methods synchronously by using a single-threaded runtime.

To be honest, I don't think it's a great solution for this case, it's doesn't make a big difference between this with the old version, and we have to handle the async/tokio runtime in the blocking client, which actually has nothing to do with tokio runtime. My intuition is using macro to wrapping them up, but the complex part of this implementation may be the function signature, we need to deal with async fn and fn.

[marioortizmanero]

To be honest, I don't think it's a great solution for this case, it's doesn't make a big difference between this with the old version, and we have to handle the async/tokio runtime in the blocking client, which actually has nothing to do with tokio runtime.

How does it not make a big difference? The implementation would only be at the async module, and the rest of the structs and functions could be taken from there as well. The thing is that with a procedural macro it seems quite complicated to cover all edge cases, I'm not sure if it's as easy as removing all the .await found and changing async fn into fn.

The macro would have to turn this:

impl Spotify {
    /// Documentation
    #[blocking]
    pub async fn track(&self, track_id: &str) -> Result<FullTrack, failure::Error> {
        // ...
    }
}

Into this:

impl Spotify {
    /// Documentation
    pub async fn track(&self, track_id: &str) -> Result<FullTrack, failure::Error> {
        // ...
    }
}

#[cfg(feature = "blocking")]
mod blocking {
    impl Spotify {
        /// Documentation
        pub fn track(&self, track_id: &str) -> Result<FullTrack, failure::Error> {
            // (awaits removed)
        }
    }
}

The runtime could instead turn the original snippet into this:

impl Spotify {
    /// Documentation
    pub async fn track(&self, track_id: &str) -> Result<FullTrack, failure::Error> {
        // ...
    }
}

#[cfg(feature = "blocking")]
mod blocking {
    impl Spotify {
        /// Documentation
        pub fn track(&self, track_id: &str) -> Result<FullTrack, failure::Error> {
            runtime.block_on(self.spotify.track(track_id));
        }
    }
}

Which at first looks more solid because it fully converts async into sync, including any edge cases. And the macro is optional in this case, just to avoid repetition. It could also reduce the binary size because there'd be less code, but I'm not fully sure about that.

The third option I've thought of is that the macro acts as a toggle, rather than having two separate Spotify. If blocking is enabled, it will be compiled without the .await. Otherwise, the original code is maintained, and no blocking module is included at all. I can't see why someone would need both async and sync clients, and this would cover the binary size problem I mentioned:

impl Spotify {
    #[cfg(not(feature = "blocking"))]
    /// Documentation
    pub async fn track(&self, track_id: &str) -> Result<FullTrack, failure::Error> {
        // ...
    }

    #[cfg(feature = "blocking")]
    /// Documentation
    pub fn track(&self, track_id: &str) -> Result<FullTrack, failure::Error> {
        runtime.block_on(self.spotify.track(track_id));
    }
}

And the final idea I'm not even sure would work is providing the async runtime inside this library itself. That way, Spotify could choose between async and blocking when it's initialized. This is what tekore does. But again, I might have not used async enough to know if that'd work.

[ramsayleung]

How does it not make a big difference? The implementation would only be at the async module, and the rest of the structs and functions could be taken from there as well.

Yes, I can get your point now, we can maintain only core codebase of asyncversion, and then implement blocking version based on async version module, instead of just maintaining two versions of copy-and-paste code, what is pretty error-prone.

The third option I've thought of is that the macro acts as a toggle, rather than having two separate Spotify. If blocking is enabled, it will be compiled without the .await. Otherwise, the original code is maintained, and no blocking module is included at all.

This is also my initial thought, but I think we have to deal with the black magic part of macro, which could be messy.

And the final idea I'm not even sure would work is providing the async runtime inside this library itself. That way, Spotify could choose between async and blocking when it's initialized.

Any prototype about this though? I have no idea how to switch between async and blocking when it's initialized, is there a way to switch between async fn and fn?