Make album, duration required for LyricsPlugin.fetch

Since at least one Backend requires album` and `duration` arguments
(`LRCLib`), the caller (`LyricsPlugin.fetch_item_lyrics`) must always
provide them.

Since they need to provided, we need to enforce this by defining them as
positional arguments.

Why is this important? I found that integrated `LRCLib` tests have been
passing, but they called `LRCLib.fetch` with values for `artist` and
`title` fields only, while the actual functionality *always* provides
values for `album` and `duration` fields too.

When I adjusted the test to provide values for the missing fields,
I found that it failed. This makes sense: Lib `album` and `duration`
filters are strict on LRCLib, so I was not surprised the lyrics could
not be found.

Thus I adjusted `LRCLib` backend implementation to only filter by each
of these fields when their values are truthy.

Author: snejus

beetsplug/lyrics.py

@@ -26,12 +26,19 @@
 import unicodedata
 import warnings
 from functools import partial
-from typing import ClassVar
+from typing import TYPE_CHECKING, ClassVar
 from urllib.parse import quote, urlencode
 
 import requests
 from unidecode import unidecode
 
+import beets
+from beets import plugins, ui
+
+if TYPE_CHECKING:
+    from beets.importer import ImportTask
+    from beets.library import Item
+
 try:
     import bs4
     from bs4 import SoupStrainer
@@ -47,10 +54,6 @@
 except ImportError:
     HAS_LANGDETECT = False
 
-
-import beets
-from beets import plugins, ui
-
 DIV_RE = re.compile(r"<(/?)div>?", re.I)
 COMMENT_RE = re.compile(r"<!--.*-->", re.S)
 TAG_RE = re.compile(r"<[^>]*>")
@@ -256,20 +259,27 @@ def fetch_url(self, url):
             self._log.debug("failed to fetch: {0} ({1})", url, r.status_code)
             return None
 
-    def fetch(self, artist, title, album=None, length=None):
-        raise NotImplementedError()
+    def fetch(
+        self, artist: str, title: str, album: str, length: int
+    ) -> str | None:
+        raise NotImplementedError
 
 
 class LRCLib(Backend):
     base_url = "https://lrclib.net/api/get"
 
-    def fetch(self, artist, title, album=None, length=None):
-        params = {
+    def fetch(
+        self, artist: str, title: str, album: str, length: int
+    ) -> str | None:
+        params: dict[str, str | int] = {
             "artist_name": artist,
             "track_name": title,
-            "album_name": album,
-            "duration": length,
         }
+        if album:
+            params["album_name"] = album
+
+        if length:
+            params["duration"] = length
 
         try:
             response = requests.get(
@@ -322,7 +332,7 @@ def encode(cls, text: str) -> str:
 
         return quote(unidecode(text))
 
-    def fetch(self, artist, title, album=None, length=None):
+    def fetch(self, artist: str, title: str, *_) -> str | None:
         url = self.build_url(artist, title)
 
         html = self.fetch_url(url)
@@ -370,7 +380,7 @@ def __init__(self, config, log):
             "User-Agent": USER_AGENT,
         }
 
-    def fetch(self, artist, title, album=None, length=None):
+    def fetch(self, artist: str, title: str, *_) -> str | None:
         """Fetch lyrics from genius.com
 
         Because genius doesn't allow accessing lyrics via the api,
@@ -501,7 +511,7 @@ class Tekstowo(DirectBackend):
     def encode(cls, text: str) -> str:
         return cls.non_alpha_to_underscore(unidecode(text.lower()))
 
-    def fetch(self, artist, title, album=None, length=None):
+    def fetch(self, artist: str, title: str, *_) -> str | None:
         if html := self.fetch_url(self.build_url(artist, title)):
             return self.extract_lyrics(html)
 
@@ -675,7 +685,7 @@ def is_page_candidate(self, url_link, url_title, title, artist):
         ratio = difflib.SequenceMatcher(None, song_title, title).ratio()
         return ratio >= typo_ratio
 
-    def fetch(self, artist, title, album=None, length=None):
+    def fetch(self, artist: str, title: str, *_) -> str | None:
         query = f"{artist} {title}"
         url = "https://www.googleapis.com/customsearch/v1?key=%s&cx=%s&q=%s" % (
             self.api_key,
@@ -885,10 +895,7 @@ def func(lib, opts, args):
             for item in items:
                 if not opts.local_only and not self.config["local"]:
                     self.fetch_item_lyrics(
-                        lib,
-                        item,
-                        write,
-                        opts.force_refetch or self.config["force"],
+                        item, write, opts.force_refetch or self.config["force"]
                     )
                 if item.lyrics:
                     if opts.printlyr:
@@ -974,15 +981,13 @@ def writerest_indexes(self, directory):
             with open(conffile, "w") as output:
                 output.write(REST_CONF_TEMPLATE)
 
-    def imported(self, session, task):
+    def imported(self, _, task: ImportTask) -> None:
         """Import hook for fetching lyrics automatically."""
         if self.config["auto"]:
             for item in task.imported_items():
-                self.fetch_item_lyrics(
-                    session.lib, item, False, self.config["force"]
-                )
+                self.fetch_item_lyrics(item, False, self.config["force"])
 
-    def fetch_item_lyrics(self, lib, item, write, force):
+    def fetch_item_lyrics(self, item: Item, write: bool, force: bool) -> None:
         """Fetch and store lyrics for a single item. If ``write``, then the
         lyrics will also be written to the file itself.
         """
@@ -991,18 +996,17 @@ def fetch_item_lyrics(self, lib, item, write, force):
             self._log.info("lyrics already present: {0}", item)
             return
 
-        lyrics = None
-        album = item.album
-        length = round(item.length)
+        lyrics_matches = []
+        album, length = item.album, round(item.length)
         for artist, titles in search_pairs(item):
-            lyrics = [
-                self.get_lyrics(artist, title, album=album, length=length)
+            lyrics_matches = [
+                self.get_lyrics(artist, title, album, length)
                 for title in titles
             ]
-            if any(lyrics):
+            if any(lyrics_matches):
                 break
 
-        lyrics = "\n\n---\n\n".join(filter(None, lyrics))
+        lyrics = "\n\n---\n\n".join(filter(None, lyrics_matches))
 
         if lyrics:
             self._log.info("fetched lyrics: {0}", item)
@@ -1027,18 +1031,20 @@ def fetch_item_lyrics(self, lib, item, write, force):
             item.try_write()
         item.store()
 
-    def get_lyrics(self, artist, title, album=None, length=None):
+    def get_lyrics(self, artist: str, title: str, *args) -> str | None:
         """Fetch lyrics, trying each source in turn. Return a string or
         None if no lyrics were found.
         """
         for backend in self.backends:
-            lyrics = backend.fetch(artist, title, album=album, length=length)
+            lyrics = backend.fetch(artist, title, *args)
             if lyrics:
                 self._log.debug(
                     "got lyrics from backend: {0}", backend.__class__.__name__
                 )
                 return _scrape_strip_cruft(lyrics, True)
 
+        return None
+
     def append_translation(self, text, to_lang):
         from xml.etree import ElementTree
 

test/plugins/test_lyrics.py

@@ -201,7 +201,7 @@ def test_backend_source(self, backend):
         """
         title = "Lady Madonna"
 
-        lyrics = backend.fetch("The Beatles", title)
+        lyrics = backend.fetch("The Beatles", title, "", 0)
 
         assert lyrics
         assert PHRASE_BY_TITLE[title] in lyrics.lower()
@@ -366,7 +366,7 @@ def backend_name(self):
     def fetch_lyrics(self, backend, requests_mock, response_data):
         requests_mock.get(lyrics.LRCLib.base_url, json=response_data)
 
-        return partial(backend.fetch, "la", "la", "la")
+        return partial(backend.fetch, "la", "la", "la", 0)
 
     @pytest.mark.parametrize(
         "response_data",