mental32 / spotify.py

🌐 API wrapper for Spotify 🎶
https://spotifypy.readthedocs.io/en/latest/
MIT License
150 stars 38 forks source link

User: Add token getters #65

Closed kmarinushkin closed 3 years ago

kmarinushkin commented 3 years ago

When I started using spotify.py module for my hobby project, I ran into a problem: First time, I authenticate from code -> Spotify gives me a token; For all future times, I authenticate from that token. The problem is: API doesn't let me get the token after my first authentication. This commit solves it.

I might miss the coding style, comments, documentation, or test coverage. It is also possible, that you would prefer to access token and refresh_token as attributes, instead of via getters. If so - please let me know, how I could improve the commit, to satisfy spotify.py quality standard.


Use-case:

This way, spotify.py module allows scripts to "remember" user, after he authenticated once.

mental32 commented 3 years ago

@kmarinushkin sorry for not noticing this earlier!

I don't think these changes are necessary, user.http.refresh_token is your refresh_token and user.http.bearer_info["access_token"] is your token.

If anything you could add a property to HTTPUserClient to sugar coat accessing the token i.e.

@property
def access_token(self):
    return self.bearer_info["access_token"]

Apart from that I don't see much that should happen, is this not enough for your use case?

I'm closing for now but feel free to discuss in this thread.

kmarinushkin commented 3 years ago

I don't think these changes are necessary, user.http.refresh_token is your refresh_token and user.http.bearer_info["access_token"] is your token.

Maybe, but it requires me to know HTTP details of Spotify API. The convenience of spotify.py is that it provides me high-level API, and so, i don't need to dive into details of low-level HTTP.

Since User class cares about tokens - having token getters are helpful for me, since i don't need to learn HTTP layer.

To sum up - i agree, it might be unnecessary; helpful nevertheless

mental32 commented 3 years ago

The token and refresh token are part of the auth flow the only thing relating this to the http API is that you perform an extra attribute access i.e. instead of user.refresh_token its user.http.refresh_token you don't need to know anything else about the http client object in order to access them.

If you really want to avoid accessing the object altogether I suggest adding two properties on the user class that accesses them for you instead, that'll preserve the "convenience" aspect of it. I'm just worried it'll add up to API bloat.

Regardless this same pattern does appear on the spotify.Client object so I'm not against a PR that does the same for the user objects in case you want to open one.

kmarinushkin commented 3 years ago

thank you, i will think of cons and pros you mentioned, and look into Client class, to see the pattern