lrclib: skip tracks where both plainLyrics and syncedLyrics are null#6767
Open
stefanvanburen wants to merge 1 commit into
Open
lrclib: skip tracks where both plainLyrics and syncedLyrics are null#6767stefanvanburen wants to merge 1 commit into
stefanvanburen wants to merge 1 commit into
Conversation
lrclib occasionally returns records with `instrumental: false` but `plainLyrics: null` and `syncedLyrics: null` — tracks that simply have no lyrics stored yet. A concrete example is Daft Punk - Nightvision (id 249829): lrclib has the record but no lyrics text for it. Before this change, `LRCLyrics.get_text()` would return `None` (the value of `self.plain`) and `LRCLib.fetch()` would pass it straight to `Lyrics(None, ...)`, causing an `AttributeError` in `Lyrics._split_lines` when it called `None.splitlines()`. Fix both layers: - `LRCLib.fetch`: guard the `Lyrics` construction with `if lyrics := item.get_text(...)` so a null result is treated as "not found" and the backend continues searching other candidates. - `Lyrics._split_lines`: add a defensive `(self.text or "")` guard so the dataclass does not crash if it is ever constructed with `text=None` through another code path. Add a test case that exercises the null/null response path.
|
Thank you for the PR! The changelog has not been updated, so here is a friendly reminder to check if you need to add an entry. |
Contributor
There was a problem hiding this comment.
Pull request overview
grug see PR try make lrclib backend not crash when API say instrumental=false but both lyrics fields null. goal good: treat that like “not found” and keep searching, plus add extra guard in Lyrics split.
Changes:
- LRCLib.fetch now skip building
Lyrics(...)whenitem.get_text(...)return null-ish. Lyrics._split_linesnow toleratetext=Noneby using empty string.- Add pytest case for lrclib response where both
plainLyricsandsyncedLyricsare null.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| test/plugins/test_lyrics.py | add coverage for lrclib response with both lyrics fields null |
| beetsplug/lyrics.py | change LRCLib.fetch to skip null lyrics text |
| beets/util/lyrics.py | guard _split_lines against text=None |
Comment on lines
400
to
+404
| if item := self.pick_best_match(candidates): | ||
| lyrics = item.get_text(self.config["synced"].get(bool)) | ||
| return Lyrics( | ||
| lyrics, self.__class__.name, f"{self.GET_URL}/{item.id}" | ||
| ) | ||
| if lyrics := item.get_text(self.config["synced"].get(bool)): | ||
| return Lyrics( | ||
| lyrics, self.__class__.name, f"{self.GET_URL}/{item.id}" | ||
| ) |
Comment on lines
106
to
109
| return [ | ||
| (m[1], m[2]) if (m := self.LINE_PARTS_PAT.match(line)) else ("", "") | ||
| for line in self.text.splitlines() | ||
| for line in (self.text or "").splitlines() | ||
| ] |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #6767 +/- ##
=======================================
Coverage 74.95% 74.95%
=======================================
Files 163 163
Lines 20893 20893
Branches 3291 3292 +1
=======================================
Hits 15661 15661
Misses 4474 4474
Partials 758 758
🚀 New features to boost your workflow:
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
lrclib occasionally returns records with
instrumental: falsebutplainLyrics: nullandsyncedLyrics: null— tracks that simply have no lyrics stored yet. A concrete example is Daft Punk - Nightvision (id 249829): lrclib has the record but no lyrics text for it:Before this change,
LRCLyrics.get_text()would returnNone(the value ofself.plain) andLRCLib.fetch()would pass it straight toLyrics(None, ...), causing anAttributeErrorinLyrics._split_lineswhen it calledNone.splitlines().Fix both layers:
LRCLib.fetch: guard theLyricsconstruction withif lyrics := item.get_text(...)so a null result is treated as "not found" and the backend continues searching other candidates.Lyrics._split_lines: add a defensive(self.text or "")guard so the dataclass does not crash if it is ever constructed withtext=Nonethrough another code path.Add a test case that exercises the null/null response path.
Documentation.Changelog.