Skip to content

Crash-resilient Tekstowo lyrics source#3995

Merged
sampsyo merged 3 commits intobeetbox:masterfrom
wisp3rwind:pr_lyrics_tekstowo_no_crashes
Jul 5, 2021
Merged

Crash-resilient Tekstowo lyrics source#3995
sampsyo merged 3 commits intobeetbox:masterfrom
wisp3rwind:pr_lyrics_tekstowo_no_crashes

Conversation

@wisp3rwind
Copy link
Copy Markdown
Member

Follow up to #3994, see the comments there, and the message for the second commit in this PR:

The previous code had the potential to crash if (when?) Tekstowo changes
their website structure sufficiently.

The new code is rather ugly due to the explicit checks after each and
every function call. Unfortunately, the alternative would be to catch a
bunch of very generic Exceptions (AttributeError, ...), since there's no
such thing as a `BeautifulSoupNotFoundError`.

The previous code had the potential to crash if (when?) Tekstowo changes
their website structure sufficiently.

The new code is rather ugly due to the explicit checks after each and
every function call. Unfortunately, the alternative would be to catch a
bunch of very generic Exceptions (AttributeError, ...), since there's no
such thing as a `BeautifulSoupNotFoundError`.
@wisp3rwind wisp3rwind force-pushed the pr_lyrics_tekstowo_no_crashes branch from 31062d5 to 0c28575 Compare July 4, 2021 08:38
@sampsyo
Copy link
Copy Markdown
Member

sampsyo commented Jul 5, 2021

This looks perfect. It's one of those cases where "monadic" handling of Nones would actually be useful… but in any case, this seems like the right thing to do! Thanks!!!

@sampsyo sampsyo merged commit 0f9ffee into beetbox:master Jul 5, 2021
@wisp3rwind wisp3rwind deleted the pr_lyrics_tekstowo_no_crashes branch July 5, 2021 13:53
@wisp3rwind
Copy link
Copy Markdown
Member Author

It's one of those cases where "monadic" handling of Nones would actually be useful…

That's a concept I've only been confronted with pretty recently, but I didn't recognize the pattern here, so thanks for pointing it out!

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.

2 participants