Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

return None for lyrics if Tekstowo fails to extract lyrics #3994

Merged
merged 1 commit into from
Jul 3, 2021

Conversation

matlads
Copy link
Contributor

@matlads matlads commented Jul 1, 2021

Description

I experienced a failure to parse Tekstowo for song lyrics.

This patch allowed the lyrics plugin to fetch the lyrics from another provider as opposed to failing.

No bug was reported before this patch was generated

I experienced a failure to parse Tekstowo for song lyrics. 

This patch allowed the lyrics plugin to fetch the lyrics from another provider as opposed to failing.
Copy link
Member

@wisp3rwind wisp3rwind left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me; see the inline comment for a minor improvement to make. If you'd like you could also add your credits to the changelog at docs/changelog.rst.

No bug was reported before this patch was generated

That's fine! As mentioned in the other pull request, it would however be great if you could briefly explain in the pull request how and why the code used to be wrong, as opposed to only saying that it failed. That would help a lot with review.

@@ -483,7 +483,10 @@ def extract_lyrics(self, html):
if not soup:
return None

return soup.find("div", class_="song-text").get_text()
c = soup.find("div", class_="song-text")
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Small nitpick: Better rename this variable to something meaningful, e.g. lyrics_div (which is a name also used by another lyrics source).

@wisp3rwind
Copy link
Member

As a side note, the tekstowo provider appears to have more issues of this kind:

beets/beetsplug/lyrics.py

Lines 463 to 465 in efcc5b3

song_rows = soup.find("div", class_="content"). \
find("div", class_="card"). \
find_all("div", class_="box-przeboje")

will crash as soon as the website is changed in such a way that this tag hierarchy is modified one of the find's returns None`.

@sampsyo
Copy link
Member

sampsyo commented Jul 1, 2021

Hmm, that's a good point, @wisp3rwind—maybe we were a little hasty with enabling this source by default and would be better off leaving it as optional until it's better tested?

@wisp3rwind
Copy link
Member

Hmm, that's a good point, @wisp3rwind—maybe we were a little hasty with enabling this source by default and would be better off leaving it as optional until it's better tested?

Actually, the class Tekstowo seems pretty simple, and on a first glance, in other places this None checks are in place. So I'd be fine with keeping it enabled. I'll go ahead, merge this PR and follow up with another one for these missing checks.

@wisp3rwind wisp3rwind merged commit ed695f2 into beetbox:master Jul 3, 2021
@wisp3rwind
Copy link
Member

Thanks @matlads for pointing out this issue 🚀

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants