[PR #120] [CLOSED] Blocking module cleanup #257

New issue

Closed

opened 2026-02-27 20:23:59 +03:00 by kerem · 0 comments

kerem commented

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

Owner

📋 Pull Request Information

Original PR: https://github.com/ramsayleung/rspotify/pull/120
Author: @marioortizmanero
Created: 8/29/2020
Status: ❌ Closed

Base: master ← Head: blocking-cleanup

📝 Commits (10+)

e4f85f8 First attempt with Deref
d0e7a5f Formatting and Cargo.toml fix
279c138 Implemented run_blocking macro
7532b3f Fix tokio import for tests, small cleanup
e783eae Implemented macro for a single function
d3e821c Now with docs too
9c783dd Single impl block, documentation
675aeb7 More clear formatting
578c13b Applied to all endpoints with a vim macro!
2c29637 Fixed docstrings

📊 Changes

10 files changed (+1067 additions, -3146 deletions)

View changed files

📝 CHANGELOG.md (+2 -0)
📝 Cargo.toml (+4 -2)
📝 src/blocking/client.rs (+27 -1946)
📝 src/blocking/mod.rs (+8 -0)
📝 src/blocking/oauth2.rs (+66 -422)
📝 src/blocking/util.rs (+28 -120)
📝 src/client.rs (+846 -642)
📝 src/lib.rs (+2 -0)
➕ src/macros.rs (+70 -0)
📝 src/oauth2.rs (+14 -14)

📄 Description

This is a WIP implementation of #112.

It removes duplicated implementations in the blocking module. I've found three possible cases of how this implementation can be hidden, after creating a wrapper in the blocking module like so:

#[derive(Debug, Clone, Deref)]
pub struct Spotify(crate::client::Spotify);

Simple methods that can be dereferenced from the original type: some can be automatically taken care of by Deref and DerefMut. For example, rspotify::oauth2::SpotifyClientCredentials::is_token_expired. These will dereference the blocking wrapper into the original async struct, and call the async struct's method. These traits are automatically implemented with the derive_deref crate, which is quite handy for our use case. This part avoids lots repetitive code but might cause issues with the typing system, since this dereference is only wanted to call the underlying type's methods. Thus, this case could be removed, writing the rest of the underlying methods like so:

fn is_token_expired(&self, token_info: &TokenInfo) -> bool {
    self.0.is_token_expired(token_info)
}

Methods that return Self: if this was left to Deref, the method would return the underlying type, not the blocking wrapper type. For example, rspotify::blocking::Spotify::default. Thus, this requires to write the function signature again in the blocking module, along with some boilerplate code. This boilerplate code may vary, but it's usually got to do with the builder pattern, and looks something like this:

pub fn access_token(mut self, access_token: &str) -> Self {
    self.0 = self.0.access_token(access_token);
    self
}

Async methods: these methods require applying some specific code in order to run them with a blocking runtime. For example, rspotify::blocking::Spotify::search. This can be done in different ways, which is discussed in #112, but for this PR I used the simplest one for now:

fn with_block_on_handle() -> Result<String, reqwest::Error> {
    RT.handle().block_on(async move {
        original().await
    })
}

I'm not sure if the async -> sync conversion with the global runtime can be done automatically with a macro, because some of these methods have parameters, and I'm not sure how that would work in a macro. At most we could have one that does this:

fn with_block_on_handle() -> Result<String, reqwest::Error> {
    blocking! {
        original()
    }
}

Which is still an improvement. NOTE: this is now implemented in the src/macros.rs file.

I still have to figure out if the documentation can be copied from the original method.

I only implemented the conversion for the search and me methods in Spotify because I only wanted to show how I think it could be done. These can be tested with the available examples (as a note, the me example doesn't work at all, but this might be because either the example or the original implementation are bugged, I'll have to look into it):

$ cargo run --example search --features="env-file blocking"

What do you think?

TODO:

Choose the best async to sync method
Clean up macro, make it as flexlible as possible and document it
Hide the macros outside the crate (maybe [#[doc(hidden)]?)
Does the macro work for generic functions? Make it work for these as well if it doesn't.
Might be unnecessary, but the macro could also accept other procedural macros applied to the method.
Apply macro to all endpoints in client.rs
Apply macro to structs in oauth.rs
Remove pub(in crate) from SpotifyClientCredentials and SpotifyOAuth

_{🔄 This issue represents a GitHub Pull Request. It cannot be merged through Gitea due to API limitations.}

## 📋 Pull Request Information **Original PR:** https://github.com/ramsayleung/rspotify/pull/120 **Author:** [@marioortizmanero](https://github.com/marioortizmanero) **Created:** 8/29/2020 **Status:** ❌ Closed **Base:** `master` ← **Head:** `blocking-cleanup` --- ### 📝 Commits (10+) - [`e4f85f8`](https://github.com/ramsayleung/rspotify/commit/e4f85f8286d496f2f2ba30c9ad882e3c4bc3f2f5) First attempt with Deref - [`d0e7a5f`](https://github.com/ramsayleung/rspotify/commit/d0e7a5f4c6f7482b647eb71e8117a389610dd152) Formatting and Cargo.toml fix - [`279c138`](https://github.com/ramsayleung/rspotify/commit/279c1387ac0f91dfa627f9dc90ca230b8fe90f6e) Implemented run_blocking macro - [`7532b3f`](https://github.com/ramsayleung/rspotify/commit/7532b3f60eb4271fa4fba877f4137f55d9a784e1) Fix tokio import for tests, small cleanup - [`e783eae`](https://github.com/ramsayleung/rspotify/commit/e783eae34ec135aa6389e8ca22026b3090a3dc69) Implemented macro for a single function - [`d3e821c`](https://github.com/ramsayleung/rspotify/commit/d3e821c5ddb72e348e63a5bad5e7df60b133c083) Now with docs too - [`9c783dd`](https://github.com/ramsayleung/rspotify/commit/9c783dd0db967e5128eeeb200b3c6c0e669bd7f6) Single impl block, documentation - [`675aeb7`](https://github.com/ramsayleung/rspotify/commit/675aeb7d0567269c40f65880f8f55ef0e592d8c8) More clear formatting - [`578c13b`](https://github.com/ramsayleung/rspotify/commit/578c13ba92f1242efaa99053765cc694079dc7f6) Applied to all endpoints with a vim macro! - [`2c29637`](https://github.com/ramsayleung/rspotify/commit/2c2963744a83b8afb7b207598e330c35ea91c8ce) Fixed docstrings ### 📊 Changes **10 files changed** (+1067 additions, -3146 deletions) <details> <summary>View changed files</summary> 📝 `CHANGELOG.md` (+2 -0) 📝 `Cargo.toml` (+4 -2) 📝 `src/blocking/client.rs` (+27 -1946) 📝 `src/blocking/mod.rs` (+8 -0) 📝 `src/blocking/oauth2.rs` (+66 -422) 📝 `src/blocking/util.rs` (+28 -120) 📝 `src/client.rs` (+846 -642) 📝 `src/lib.rs` (+2 -0) ➕ `src/macros.rs` (+70 -0) 📝 `src/oauth2.rs` (+14 -14) </details> ### 📄 Description This is a WIP implementation of #112. It removes duplicated implementations in the blocking module. I've found three possible cases of how this implementation can be hidden, after creating a wrapper in the blocking module like so: ```rust #[derive(Debug, Clone, Deref)] pub struct Spotify(crate::client::Spotify); ``` * **Simple methods that can be dereferenced from the original type**: some can be automatically taken care of by `Deref` and `DerefMut`. For example, `rspotify::oauth2::SpotifyClientCredentials::is_token_expired`. These will dereference the blocking wrapper into the original async struct, and call the async struct's method. These traits are automatically implemented with the `derive_deref` crate, which is quite handy for our use case. This part avoids lots repetitive code but might cause issues with the typing system, since this dereference is only wanted to call the underlying type's methods. Thus, this case could be removed, writing the rest of the underlying methods like so: ```rust fn is_token_expired(&self, token_info: &TokenInfo) -> bool { self.0.is_token_expired(token_info) } ``` * **Methods that return `Self`**: if this was left to `Deref`, the method would return the *underlying type*, not the blocking wrapper type. For example, `rspotify::blocking::Spotify::default`. Thus, this requires to write the *function signature* again in the blocking module, along with some boilerplate code. This boilerplate code may vary, but it's usually got to do with the builder pattern, and looks something like this: ```rust pub fn access_token(mut self, access_token: &str) -> Self { self.0 = self.0.access_token(access_token); self } ``` * **Async methods**: these methods require applying some specific code in order to run them with a blocking runtime. For example, `rspotify::blocking::Spotify::search`. This can be done in different ways, which is discussed in #112, but for this PR I used the simplest one for now: ```rust fn with_block_on_handle() -> Result<String, reqwest::Error> { RT.handle().block_on(async move { original().await }) } ``` I'm not sure if the async -> sync conversion with the global runtime can be done automatically with a macro, because some of these methods have parameters, and I'm not sure how that would work in a macro. At most we could have one that does this: ```rust fn with_block_on_handle() -> Result<String, reqwest::Error> { blocking! { original() } } ``` Which is still an improvement. NOTE: this is now implemented in the `src/macros.rs` file. I still have to figure out if the documentation can be copied from the original method. I only implemented the conversion for the `search` and `me` methods in `Spotify` because I only wanted to show how I think it could be done. These can be tested with the available examples (as a note, the `me` example doesn't work at all, but this might be because either the example or the original implementation are bugged, I'll have to look into it): ```bash $ cargo run --example search --features="env-file blocking" ``` What do you think? **TODO**: - [ ] Choose the best async to sync method - [ ] Clean up macro, make it as flexlible as possible and document it - [ ] Hide the macros outside the crate (maybe `[#[doc(hidden)]`?) - [ ] Does the macro work for generic functions? Make it work for these as well if it doesn't. - [ ] Might be unnecessary, but the macro could also accept other procedural macros applied to the method. - [ ] Apply macro to all endpoints in `client.rs` - [ ] Apply macro to structs in `oauth.rs` - [ ] Remove `pub(in crate)` from `SpotifyClientCredentials` and `SpotifyOAuth` --- <sub>🔄 This issue represents a GitHub Pull Request. It cannot be merged through Gitea due to API limitations.</sub>