lyrics: don't match LRCLib entries that have no lyrics - #6900
lyrics: don't match LRCLib entries that have no lyrics#6900davidbhoward wants to merge 2 commits into
Conversation
LRCLib stores track metadata independently of the lyrics themselves, so an
entry can come back with both `plainLyrics` and `syncedLyrics` null while
`instrumental` is False. `LRCLyrics.is_valid` accepted such an entry as a
match on duration alone, and `get_text` then returned `self.plain`, i.e.
None, despite being annotated `-> str`.
The None propagated into `Lyrics`, and the first access of its text raised
AttributeError: 'NoneType' object has no attribute 'splitlines'
`beet lyrics` surfaced this per track, but during an import the exception
escaped the pipeline stage and aborted the entire run: one such track
stranded every file queued behind it.
Treat an entry with no lyrics text as not a match, so the search moves on to
other candidates and backends and ultimately reports that no lyrics were
found. That is deliberately distinct from an instrumental track, where "no
lyrics" is itself the answer and the existing `instrumental` handling still
applies. Marking these as instrumental would assert something the API
response does not tell us.
Also fall back to synced lyrics when only `plainLyrics` is null, rather than
discarding lyrics we do have, and correct the annotations: `plainLyrics` and
`LRCLyrics.plain` are both nullable.
|
Full disclosure on where this came from: I have not personally reviewed the code in this The repro in the description is a real response from the LRCLib API, so at minimum the Posting it in case it helps. Take it or leave it, no hard feelings either way. |
|
The two failures in the ubuntu 3.10 job look unrelated to this change. I reverted Both are live network tests in TestLyricsSources. The lyricsmania one is the Google These only run when lyrics source changes, so I think this PR just happened to be what |
|
Is this still an issue on the master branch? I think #6864 should have fixed this already. |
Thanks for pointing at #6864. I checked and I don't think it covers this one: #6864 is The difference is where the None comes from. #6864 guards Lyrics.from_item(), which handles get_text() falls through to The repro in the PR description is a real LRCLib response and it still fails on current |
|
Thanks for the clarification! Makes sense to me. |
| # Unreachable for a candidate that passed :attr:`is_valid`, which | ||
| # requires some lyrics to be available. Kept so that this method always | ||
| # returns a string. | ||
| return INSTRUMENTAL_LYRICS |
There was a problem hiding this comment.
This is confusing. Return an empty string instead.
|
@davidbhoward I fixed |
|
OK will do. I'm on vacation but will do it when I get back next week. Thank you. |
|
|
||
| @staticmethod | ||
| def _format_synced(synced: str) -> str: | ||
| return "\n".join(map(str.strip, synced.splitlines())) |
There was a problem hiding this comment.
If the goal of this is to format synced lyrics for use as plain text lyrics, the timestamps should probably be stripped
4 participants
Description
LRCLib stores track metadata independently of the lyrics themselves, so an entry can come back with both
plainLyricsandsyncedLyricsnull whileinstrumentalis stillFalse.LRCLyrics.is_validaccepted such an entry as a match on duration alone, andget_textthen returnedself.plain, i.e.None, despite being annotated-> str. TheNonepropagated intoLyrics, and the first access of its text raised:beet lyricssurfaces this per track, but during an import the exception escapes the pipeline stage and aborts the entire run, so one such track strands every file queued behind it. Instrumental-heavy material (lo-fi, game soundtracks, ambient) hits it often.Reproducing
Real response from the public API,
https://lrclib.net/api/search?track_name=Anther&artist_name=Blue%20Wednesday:{ "id": 37048543, "trackName": "Anther", "artistName": "Blue Wednesday", "duration": 189.0, "instrumental": false, "plainLyrics": null, "syncedLyrics": null }Against
master, with no configuration involved:Changes
instrumentalhandling is unchanged. Marking these as instrumental would assert something the API response does not tell us: the lyrics may simply not have been contributed yet.plainLyricsnow falls back to synced lyrics rather than discarding lyrics that are present.LRCLibAPI.Item.plainLyricsandLRCLyrics.plainare annotated as nullable, matching what the API actually returns.Tests
Two cases added to
TestLRCLibLyrics, both of which fail onmasterand pass with this change:none: no lyrics text despite instrumental being Falsetest_null_plain_lyrics_falls_back_to_syncedFull
test/plugins/test_lyrics.pysuite passes (127 passed, 16 skipped), along withruff check,ruff format --check, andmypy.