search

ramsayleung / rspotify

Spotify Web API SDK implemented on Rust
MIT License
639 stars 123 forks source link

Remove unnecessary mutable references in clients #321

Closed Spanfile closed 2 years ago

Spanfile commented 2 years ago

Description

OAuthClient::request_token takes self as &mut, yet all of its implementors are designed for an async environment, where inner mutability for their fields is required through Arc<Mutex<_>>, as is the case for the token fields, thus self doesn't actually have to be a mutable reference. By making the reference immutable, it is much more comfortable to use the clients in application code, since there's no need to wrap them in e.g. a Mutex or an RwLock only for the one case where a mutable self is needlessly required.

However, AuthCodePkceSpotify::get_authorize_url is still left as is to take self as &mut since it has to set its verifier field, and converting that field to Arc<Mutex<_>> and converting the function into async would be a breaking change, hence I've left it out of this PR.

Motivation and Context

I've been using the library recently and ran into this ergonomics issue.

Dependencies

None.

Type of change

How has this been tested?

The crate test suite ran succesfully. I removed one unneeded mut in tests/test_with_credential.rs.

Is this change properly documented?

I'm unsure if this is a breaking change, since existing application code will still work, it would just be beneficial for them to adapt to not requiring mutability anymore. If someone has implemented OAuthClient for their own type, though, it will be a breaking change since the trait signature has changed.

marioortizmanero commented 2 years ago

Hey, thanks for the contribution!

Yes, you're completely right. I think I didn't end up doing this back then because it just felt weird to do spotify.prompt_for_token without it being mutable. The user could think that said function actually doesn't modify the internal token.

But it doesn't make sense to keep it mutable because it disallows concurrency, so we should definitely merge this. You'll need to update the tests and examples, though, as mut is used in many other places.

Spanfile commented 2 years ago

Thank you for your feedback. I've gone ahead and removed all unnecessary muts in the examples.

marioortizmanero commented 2 years ago

Merging this! Thanks again.