Skip to content

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.

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