Fix #3535 - Genius Null Check #3554
Conversation
There was a problem hiding this comment.
Looking good so far; thanks!!
Would you mind trimming down some of the bulky markup in the sample HTML file? There are two reasons to do this: (1) saving space, and (2) avoiding intellectual property infringement. That is, while there are no actual lyrics in there, there is plenty of stuff that could be construed as "owned" by Genius. Is it possible to have a meaningful test with only a minimal/stripped-down skeleton of the real page?
Also, would you mind adding a quick note about the fix to the changelog (under docs)?
beetsplug/lyrics.py
Outdated
| lyrics = html.find("div", class_="lyrics").get_text() | ||
| try: | ||
| lyrics = html.find("div", class_="lyrics").get_text() | ||
| except AttributeError as exc: |
There was a problem hiding this comment.
Cool! I think this might be a little clearer (and not require the code-like reasoning that currently appears in a comment) if we instead check specifically for None instead of generically for a missing attribute:
lyrics_div = html.find(...)
if lyrics_div is None:
self._log...
return None
lyrics = lyrics_div.get_text()What do you think about that minor restructuring?
There was a problem hiding this comment.
Seems like it should have the same functionality, I'll rewrite it and test it again to make sure! Checking for None in general seems like a better failsafe, I think I was just using try-except because that's what the other error checking seemed to be doing.
test/test_lyrics.py
Outdated
| try: | ||
| self.assertEqual(genius.lyrics_from_song_api_path('/nolyric'), | ||
| None) | ||
| except AttributeError: |
There was a problem hiding this comment.
Instead of catching the exception and assessing, I think we can just let the exception through. That is, you can remove the try/except handler here completely—the test will error out if the bug exists.
|
@sampsyo made the recommended changes, trimmed the example html, and made a small note in the changelog. |
|
Awesome; looks great! Thanks for taking care of this! |
Added the null check with a unit test and sample Genius page.