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

reconfigured types and added album, added retries and etc #162

Closed allerter closed 3 years ago

allerter commented 3 years ago
allerter commented 3 years ago

For all the methods in the API except web_page there is an overlap. Also methods like search_song and search_artist, artist.add_song and artist.song call these overlapping methods. So now the client will call the API by default if the user doesn't explicitly set public_api in the parameters. But that could really inconvenience token-less users since for all methods that call the overlapping methods they'd need to set the public_api parameter. Therefore I added a Genius.public_api attribute which makes it easier to have all the calls go to the public API. The body of methods like genius.song look like this now:

if public_api or self.public_api:
    return super(PublicAPI, self).song(song_id, text_format)
else:
    try:
        return super().song(song_id, text_format)
    except TokenRequiredError:
        raise TokenRequiredError(public_api=True)

the TokenRequiredError(public_api=True) will re-raise the exception with a message different from the default one. So if a token-less user calls genius.song(1) when genius.public_api=False -which is the default value- their traceback will look like this:

Traceback (most recent call last):
  File "...genius.py", line 445, in song
    return super().song(song_id, text_format)
  File "...base.py", line 101, in wrapper
    raise TokenRequiredError()
lyricsgenius.exceptions.TokenRequiredError: This method requires an access token.

During handling of the above exception, another exception occurred:

Traceback (most recent call last):
  File "...test.py", line 7, in <module>
    genius.song(1)
  File ...genius.py", line 447, in song
    raise TokenRequiredError(public_api=True)
lyricsgenius.exceptions.TokenRequiredError: Using this method with the developers API needs an access token. Get an access token or use the public API by setting public_api=True in method parameters or Genius.public_api=True

Instead of re-raising the exception, we could raise the exception with the right message the first time (I've commented that solution in check_token. But in that case, we would have to check the public_api or self.public_api condition twice: once in check_token and the other inside the function. So should we re-raise or re-check? I'd like to know your thoughts on this.

allerter commented 3 years ago

Another way would be to use the check_token decorator only with the API (like the re-raise solution), and have the body of the overlapping methods like this:

if public_api or self.public_api:
    return super(PublicAPI, self).song(song_id, text_format)
else:
    if self.access_token is None:
        raise TokenRequiredError(public_api=True)
    return super().song(song_id, text_format)
allerter commented 3 years ago

The lyrics for the song used in the test_song_annotations had recently changed, and the test failed because of that. So I updated the test and the method's return value. It used to return a (fragment, annotations in a generator), but now it's (fragment, annotations in a list) as I had intended in the first place when I first updated that method.

As for checking the token in the overlapping methods, I decided to go with checking the token inside the method body, as explained here

allerter commented 3 years ago

Hi, @johnwmillr. Please review this when you get a chance.

johnwmillr commented 3 years ago

Hi, @Allerter. It's been a busy few weeks for me, but I do plan to review this PR. Thanks for your work on it! I'll get to work reviewing it this week.