From 91ece4c44a6c1cd73aa1056fb3eea19e098e6c07 Mon Sep 17 00:00:00 2001 From: Henry Date: Sun, 7 Dec 2025 14:37:12 -0800 Subject: [PATCH] Fix #6068 - Multivalue fields are now supported & tested. --- beetsplug/discogs.py | 212 +++++++++++++++++++++++++---------- docs/changelog.rst | 1 + test/plugins/test_discogs.py | 103 ++++++++++++++--- 3 files changed, 238 insertions(+), 78 deletions(-) diff --git a/beetsplug/discogs.py b/beetsplug/discogs.py index 6941cf891..eb8465960 100644 --- a/beetsplug/discogs.py +++ b/beetsplug/discogs.py @@ -18,6 +18,7 @@ python3-discogs-client library. from __future__ import annotations +import copy import http.client import json import os @@ -105,6 +106,24 @@ class Track(TypedDict): sub_tracks: NotRequired[list[Track]] +class ArtistInfo(TypedDict): + artist: str + artists: list[str] + artist_credit: str + artists_credit: list[str] + artist_id: str + artists_ids: list[str] + + +class AlbumArtistInfo(ArtistInfo): + albumartist: str + albumartists: list[str] + albumartist_credit: str + albumartists_credit: list[str] + albumartist_id: str + albumartists_ids: list[str] + + class DiscogsPlugin(MetadataSourcePlugin): def __init__(self): super().__init__() @@ -261,7 +280,6 @@ class DiscogsPlugin(MetadataSourcePlugin): for track in album.tracks: if track.track_id == track_id: return track - return None def get_albums(self, query: str) -> Iterable[AlbumInfo]: @@ -346,6 +364,121 @@ class DiscogsPlugin(MetadataSourcePlugin): artist, artist_id = self.get_artist(artist_list, join_key="join") return self.strip_disambiguation(artist), artist_id + def build_albumartistinfo(self, artists: list[Artist]) -> AlbumArtistInfo: + info = self.build_artistinfo(artists, album_artist=True) + albumartist: AlbumArtistInfo = { + **info, + "albumartist": info["artist"], + "albumartist_id": info["artist_id"], + "albumartists": info["artists"], + "albumartists_ids": info["artists_ids"], + "albumartist_credit": info["artist_credit"], + "albumartists_credit": info["artists_credit"], + } + return albumartist + + def build_artistinfo( + self, + given_artists: list[Artist], + given_info: ArtistInfo | None = None, + album_artist: bool = False, + ) -> ArtistInfo: + """Iterates through a discogs result and builds + up the artist fields. Does not contribute to + artist_sort as Discogs does not define that. + + :param artists: A list of Discogs Artist objects + + :param album_artist: If building an album artist, + we need to account for the album_artist anv parameter. + :return an ArtistInfo dictionary. + """ + info: ArtistInfo = { + "artist": "", + "artist_id": "", + "artists": [], + "artists_ids": [], + "artist_credit": "", + "artists_credit": [], + } + if given_info: + info = copy.deepcopy(given_info) + + a_anv: bool = self.config["anv"]["artist"].get(bool) + ac_anv: bool = self.config["anv"]["artist_credit"].get(bool) + aa_anv: bool = self.config["anv"]["album_artist"].get(bool) + feat_str: str = f" {self.config['featured_string'].get(str)} " + + artist = "" + artist_anv = "" + artists: list[str] = [] + artists_anv: list[str] = [] + + join = "" + featured_flag = False + # Iterate through building the artist strings + for a in given_artists: + # Get the artist name + name = self.strip_disambiguation(a["name"]) + discogs_id = a["id"] + anv = a.get("anv", name) + role = a.get("role", "").lower() + # Check if the artist is Various + if name.lower() == "various": + name = config["va_name"].as_str() + anv = name + + if "featuring" in role: + if not featured_flag: + artist += feat_str + artist_anv += feat_str + artist += name + artist_anv += anv + featured_flag = True + else: + artist = self._join_artist(artist, name, join) + artist_anv = self._join_artist(artist_anv, anv, join) + elif role and "featuring" not in role: + continue + else: + artist = self._join_artist(artist, name, join) + artist_anv = self._join_artist(artist_anv, anv, join) + artists.append(name) + artists_anv.append(anv) + # Only the first ID is set for the singular field + if not info["artist_id"]: + info["artist_id"] = discogs_id + info["artists_ids"].append(discogs_id) + # Update join for the next artist + join = a.get("join", "") + # Assign fields as necessary + if (a_anv and not album_artist) or (aa_anv and album_artist): + info["artist"] += artist_anv + info["artists"] += artists_anv + else: + info["artist"] += artist + info["artists"] += artists + + if ac_anv: + info["artist_credit"] += artist_anv + info["artists_credit"] += artists_anv + else: + info["artist_credit"] += artist + info["artists_credit"] += artists + return info + + def _join_artist(self, base: str, artist: str, join: str) -> str: + # Expand the artist field + if not base: + base = artist + else: + if join: + base += f" {join} " + else: + base += ", " + base += artist + return base + def get_album_info(self, result: Release) -> AlbumInfo | None: """Returns an AlbumInfo object for a discogs Release object.""" # Explicitly reload the `Release` fields, as they might not be yet @@ -375,11 +508,8 @@ class DiscogsPlugin(MetadataSourcePlugin): return None artist_data = [a.data for a in result.artists] - album_artist, album_artist_id = self.get_artist_with_anv(artist_data) - album_artist_anv, _ = self.get_artist_with_anv( - artist_data, use_anv=True - ) - artist_credit = album_artist_anv + # Information for the album artist + albumartist: AlbumArtistInfo = self.build_albumartistinfo(artist_data) album = re.sub(r" +", " ", result.title) album_id = result.data["id"] @@ -388,18 +518,11 @@ class DiscogsPlugin(MetadataSourcePlugin): # information and leave us with skeleton `Artist` objects that will # each make an API call just to get the same data back. tracks = self.get_tracks( - result.data["tracklist"], - (album_artist, album_artist_anv, album_artist_id), + result.data["tracklist"], self.build_artistinfo(artist_data) ) - # Assign ANV to the proper fields for tagging - if not self.config["anv"]["artist_credit"]: - artist_credit = album_artist - if self.config["anv"]["album_artist"]: - album_artist = album_artist_anv - # Extract information for the optional AlbumInfo fields, if possible. - va = result.data["artists"][0].get("name", "").lower() == "various" + va = albumartist["albumartist"] == config["va_name"].as_str() year = result.data.get("year") mediums = [t.medium for t in tracks] country = result.data.get("country") @@ -431,11 +554,7 @@ class DiscogsPlugin(MetadataSourcePlugin): cover_art_url = self.select_cover_art(result) # Additional cleanups - # (various artists name, catalog number, media, disambiguation). - if va: - va_name = config["va_name"].as_str() - album_artist = va_name - artist_credit = va_name + # (catalog number, media, disambiguation). if catalogno == "none": catalogno = None # Explicitly set the `media` for the tracks, since it is expected by @@ -458,9 +577,7 @@ class DiscogsPlugin(MetadataSourcePlugin): return AlbumInfo( album=album, album_id=album_id, - artist=album_artist, - artist_credit=artist_credit, - artist_id=album_artist_id, + **albumartist, # Unpacks values to satisfy the keyword arguments tracks=tracks, albumtype=albumtype, va=va, @@ -478,7 +595,7 @@ class DiscogsPlugin(MetadataSourcePlugin): data_url=data_url, discogs_albumid=discogs_albumid, discogs_labelid=labelid, - discogs_artistid=album_artist_id, + discogs_artistid=albumartist["albumartist_id"], cover_art_url=cover_art_url, ) @@ -503,7 +620,7 @@ class DiscogsPlugin(MetadataSourcePlugin): def _process_clean_tracklist( self, clean_tracklist: list[Track], - album_artist_data: tuple[str, str, str | None], + albumartistinfo: ArtistInfo, ) -> tuple[ list[TrackInfo], dict[int, str], @@ -531,7 +648,7 @@ class DiscogsPlugin(MetadataSourcePlugin): divisions += next_divisions del next_divisions[:] track_info, medium, medium_index = self.get_track_info( - track, index, divisions, album_artist_data + track, index, divisions, albumartistinfo ) track_info.track_alt = track["position"] tracks.append(track_info) @@ -565,7 +682,7 @@ class DiscogsPlugin(MetadataSourcePlugin): def get_tracks( self, tracklist: list[Track], - album_artist_data: tuple[str, str, str | None], + albumartistinfo: ArtistInfo, ) -> list[TrackInfo]: """Returns a list of TrackInfo objects for a discogs tracklist.""" try: @@ -578,7 +695,7 @@ class DiscogsPlugin(MetadataSourcePlugin): self._log.error("uncaught exception in coalesce_tracks: {}", exc) clean_tracklist = tracklist processed = self._process_clean_tracklist( - clean_tracklist, album_artist_data + clean_tracklist, albumartistinfo ) ( tracks, @@ -754,16 +871,11 @@ class DiscogsPlugin(MetadataSourcePlugin): track: Track, index: int, divisions: list[str], - album_artist_data: tuple[str, str, str | None], + albumartistinfo: ArtistInfo, ) -> tuple[TrackInfo, str | None, str | None]: """Returns a TrackInfo object for a discogs track.""" - artist, artist_anv, artist_id = album_artist_data - artist_credit = artist_anv - if not self.config["anv"]["artist_credit"]: - artist_credit = artist - if self.config["anv"]["artist"]: - artist = artist_anv + artistinfo = albumartistinfo.copy() title = track["title"] if self.config["index_tracks"]: @@ -775,39 +887,19 @@ class DiscogsPlugin(MetadataSourcePlugin): # If artists are found on the track, we will use those instead if artists := track.get("artists", []): - artist, artist_id = self.get_artist_with_anv( - artists, self.config["anv"]["artist"] - ) - artist_credit, _ = self.get_artist_with_anv( - artists, self.config["anv"]["artist_credit"] - ) + artistinfo = self.build_artistinfo(artists) + length = self.get_track_length(track["duration"]) # Add featured artists if extraartists := track.get("extraartists", []): - featured_list = [ - artist - for artist in extraartists - if "Featuring" in artist["role"] - ] - featured, _ = self.get_artist_with_anv( - featured_list, self.config["anv"]["artist"] - ) - featured_credit, _ = self.get_artist_with_anv( - featured_list, self.config["anv"]["artist_credit"] - ) - if featured: - artist += f" {self.config['featured_string']} {featured}" - artist_credit += ( - f" {self.config['featured_string']} {featured_credit}" - ) + artistinfo = self.build_artistinfo(extraartists, artistinfo) + return ( TrackInfo( title=title, track_id=track_id, - artist_credit=artist_credit, - artist=artist, - artist_id=artist_id, + **artistinfo, length=length, index=index, ), diff --git a/docs/changelog.rst b/docs/changelog.rst index 957574fdf..66a482d11 100644 --- a/docs/changelog.rst +++ b/docs/changelog.rst @@ -26,6 +26,7 @@ New features: - :doc:`plugins/mbpseudo`: Add a new `mbpseudo` plugin to proactively receive MusicBrainz pseudo-releases as recommendations during import. - Added support for Python 3.13. +- :doc:`plugins/discogs`: Added support for multi value fields. :bug:`6068` Bug fixes: diff --git a/test/plugins/test_discogs.py b/test/plugins/test_discogs.py index fd820ab43..c11148c13 100644 --- a/test/plugins/test_discogs.py +++ b/test/plugins/test_discogs.py @@ -409,7 +409,9 @@ class DGAlbumInfoTest(BeetsTestCase): ) d = DiscogsPlugin().get_album_info(release) assert d.artist == "ARTIST NAME & OTHER ARTIST" + assert d.artists == ["ARTIST NAME", "OTHER ARTIST"] assert d.tracks[0].artist == "TEST ARTIST" + assert d.tracks[0].artists == ["TEST ARTIST"] assert d.label == "LABEL NAME" def test_strip_disambiguation_false(self): @@ -448,35 +450,62 @@ class DGAlbumInfoTest(BeetsTestCase): ) d = DiscogsPlugin().get_album_info(release) assert d.artist == "ARTIST NAME (2) & OTHER ARTIST (5)" + assert d.artists == ["ARTIST NAME (2)", "OTHER ARTIST (5)"] assert d.tracks[0].artist == "TEST ARTIST (5)" + assert d.tracks[0].artists == ["TEST ARTIST (5)"] assert d.label == "LABEL NAME (5)" config["discogs"]["strip_disambiguation"] = True @pytest.mark.parametrize( - "track_artist_anv,track_artist", - [(False, "ARTIST Feat. PERFORMER"), (True, "VARIATION Feat. VARIATION")], -) -@pytest.mark.parametrize( - "album_artist_anv,album_artist", - [(False, "ARTIST & SOLOIST"), (True, "VARIATION & VARIATION")], -) -@pytest.mark.parametrize( - "artist_credit_anv,track_artist_credit,album_artist_credit", + "track_artist_anv,track_artist,track_artists", [ - (False, "ARTIST Feat. PERFORMER", "ARTIST & SOLOIST"), - (True, "VARIATION Feat. VARIATION", "VARIATION & VARIATION"), + (False, "ARTIST Feat. PERFORMER", ["ARTIST", "PEFORMER"]), + (True, "VARIATION Feat. VARIATION", ["VARIATION", "VARIATION"]), + ], +) +@pytest.mark.parametrize( + "album_artist_anv,album_artist,album_artists", + [ + (False, "ARTIST & SOLOIST", ["ARTIST", "SOLOIST"]), + (True, "VARIATION & VARIATION", ["VARIATION", "VARIATION"]), + ], +) +@pytest.mark.parametrize( + ( + "artist_credit_anv,track_artist_credit," + "track_artists_credit,album_artist_credit,album_artists_credit" + ), + [ + ( + False, + "ARTIST Feat. PERFORMER", + ["ARTIST", "PEFORMER"], + "ARTIST & SOLOIST", + ["ARTIST", "SOLOIST"], + ), + ( + True, + "VARIATION Feat. VARIATION", + ["VARIATION", "VARIATION"], + "VARIATION & VARIATION", + ["VARIATION", "VARIATION"], + ), ], ) @patch("beetsplug.discogs.DiscogsPlugin.setup", Mock()) def test_anv( track_artist_anv, track_artist, + track_artists, album_artist_anv, album_artist, + album_artists, artist_credit_anv, track_artist_credit, + track_artists_credit, album_artist_credit, + album_artists_credit, ): """Test using artist name variations.""" data = { @@ -558,13 +587,21 @@ def test_anv_album_artist(): config["discogs"]["anv"]["artist_credit"] = False r = DiscogsPlugin().get_album_info(release) assert r.artist == "ARTIST" + assert r.artists == ["ARTIST"] + assert r.albumartist == "ARTIST" + assert r.albumartist_credit == "ARTIST" + assert r.albumartists == ["ARTIST"] + assert r.albumartists_credit == ["ARTIST"] assert r.artist_credit == "ARTIST" + assert r.artists_credit == ["ARTIST"] assert r.tracks[0].artist == "VARIATION" + assert r.tracks[0].artists == ["VARIATION"] assert r.tracks[0].artist_credit == "ARTIST" + assert r.tracks[0].artists_credit == ["ARTIST"] @pytest.mark.parametrize( - "track, expected_artist", + "track, expected_artist, expected_artists", [ ( { @@ -600,18 +637,25 @@ def test_anv_album_artist(): ], }, "NEW ARTIST, VOCALIST Feat. SOLOIST, PERFORMER, MUSICIAN", + ["NEW ARTIST", "VOCALIST", "SOLOIST", "PERFORMER", "MUSICIAN"], ), ], ) @patch("beetsplug.discogs.DiscogsPlugin.setup", Mock()) -def test_parse_featured_artists(track, expected_artist): +def test_parse_featured_artists(track, expected_artist, expected_artists): """Tests the plugins ability to parse a featured artist. - Initial check with one featured artist, two featured artists, - and three. Ignores artists that are not listed as featured.""" - t, _, _ = DiscogsPlugin().get_track_info( - track, 1, 1, ("ARTIST", "ARTIST CREDIT", 2) - ) + Ignores artists that are not listed as featured.""" + artistinfo = { + "artist": "ARTIST", + "artist_id": "1", + "artists": ["ARTIST"], + "artists_ids": ["1"], + "artist_credit": "ARTIST", + "artists_credit": ["ARTIST"], + } + t, _, _ = DiscogsPlugin().get_track_info(track, 1, 1, artistinfo) assert t.artist == expected_artist + assert t.artists == expected_artists @pytest.mark.parametrize( @@ -637,6 +681,29 @@ def test_get_media_and_albumtype(formats, expected_media, expected_albumtype): assert result == (expected_media, expected_albumtype) +@pytest.mark.parametrize( + "given_artists,expected_info,config_va_name", + [ + ( + [{"name": "Various", "id": "1"}], + { + "artist": "VARIOUS ARTISTS", + "artist_id": "1", + "artists": ["VARIOUS ARTISTS"], + "artists_ids": ["1"], + "artist_credit": "VARIOUS ARTISTS", + "artists_credit": ["VARIOUS ARTISTS"], + }, + "VARIOUS ARTISTS", + ) + ], +) +@patch("beetsplug.discogs.DiscogsPlugin.setup", Mock()) +def test_va_buildartistinfo(given_artists, expected_info, config_va_name): + config["va_name"] = config_va_name + assert DiscogsPlugin().build_artistinfo(given_artists) == expected_info + + @pytest.mark.parametrize( "position, medium, index, subindex", [