johnwmillr / LyricsGenius

Download song lyrics and metadata from Genius.com 🎶🎤
http://www.johnwmillr.com/scraping-genius-lyrics/
MIT License
892 stars 158 forks source link

Genius class should automatically check for token environment variable #165

Closed johnwmillr closed 3 years ago

johnwmillr commented 3 years ago

Is your feature request related to a problem? Please describe. No problem, it's just a hassle to retrieve your client access token from os each time you use lyricsgenius.

Describe the solution you'd like If the user doesn't pass a user token to Genius, the class should check the OS environment variables for GENIUS_ACCESS_TOKEN.

Describe alternatives you've considered There are no alternatives.

Additional context Probably here is where you'd modify the code.

allerter commented 3 years ago

Shouldn't the user explicitly pass the token themself? What if the user doesn't want to pass a token, or the token in the env var is invalid. This might lead to confusion. How about this:

import lyricsgenius as lg
token = lg.from_environment()

And for the Auth class:

import lyricsgenius as lg
client_id, redirect_uri, client_secret = lg.from_environment(auth=True)
johnwmillr commented 3 years ago

Yes, I'd prefer for the user to pass the token, but if they don't, checking for the environment variable could be a fallback.

allerter commented 3 years ago

The only case that could lead to confusion is that if the user passes no token, but has an invalid token in their env var, and then tries to use an endpoint using the developers API. Should we print a message in that case? Something like this:

if token is None:
    token = os.environ.get("GENIUS_ACCESS_TOKEN")
    if token is not None:
        print("Token was obtained from environment variables.")
self.access_token = token
johnwmillr commented 3 years ago

But how is an invalid token supplied from an environment variable any different than a user supplying an invalid token directly?

I agree though, a notification like you've written there would be necessary.

This issue in part relates to #162 and my preference to require an access token for any functionality that is part of the official API. Sorry I haven't gotten to your PR yet, I've been thinking about how best to incorporate public, unauthenticated methods into the package. I would prefer for the unauthenticated, public API methods only to be a part of the package when the same functionality is not offered through the authenticated API.

allerter commented 3 years ago

The difference is when the user wants to use the token-less Genius but has a token present in the env vars.

To do what you propose has some advantages and disadvantages:

Advantages:

Disadvantages:

So should we remove overlapping methods in the PublicAPI class? Even though the Genius object will inherit both API classes, the users won't have access to overlapping methods like PublicAPI.song() through Genius since API.song() will have higher priority. But they could use PublicAPI.song() if they wanted.

allerter commented 3 years ago

It's probably better for the package to rely on the API as much as possible as you said. And since the token-less branch is complete, I'll just leave it on my fork. I'll update #162 soon.