From ee5f96be78ed271c31bdef05fce746bdc9a09e38 Mon Sep 17 00:00:00 2001 From: Johann Fot Date: Sun, 7 Dec 2025 00:22:26 +0100 Subject: [PATCH] Add native support for multiple genres per album/track Simplify multi-genre implementation based on maintainer feedback (PR #6169). Changes: - Remove multi_value_genres and genre_separator config options - Replace complex sync_genre_fields() with ensure_first_value('genre', 'genres') - Update all plugins (Beatport, MusicBrainz, LastGenre) to always write genres as lists - Add automatic migration for comma/semicolon/slash-separated genre strings - Add 'beet migrate genres' command for explicit batch migration with --pretend flag - Update all tests to reflect simplified approach (44 tests passing) - Update documentation Implementation aligns with maintainer vision of always using multi-value genres internally with automatic backward-compatible sync to the genre field via ensure_first_value(), eliminating configuration complexity. Migration strategy avoids problems from #5540: - Automatic lazy migration on item access (no reimport/mbsync needed) - Optional batch migration command for user control - No endless rewrite loops due to proper field synchronization --- beets/autotag/__init__.py | 31 +++++++++ beets/autotag/hooks.py | 2 + beets/library/models.py | 3 + beets/ui/commands/__init__.py | 10 +-- beets/ui/commands/migrate.py | 98 +++++++++++++++++++++++++++ beetsplug/beatport.py | 17 +++-- beetsplug/lastgenre/__init__.py | 55 +++++++-------- beetsplug/musicbrainz.py | 5 +- docs/changelog.rst | 21 +++++- test/plugins/test_beatport.py | 9 ++- test/plugins/test_lastgenre.py | 98 ++++++++++++++++++--------- test/plugins/test_musicbrainz.py | 4 +- test/test_autotag.py | 113 +++++++++++++++++++++++++++++++ test/test_library.py | 36 ++++++++-- 14 files changed, 421 insertions(+), 81 deletions(-) create mode 100644 beets/ui/commands/migrate.py diff --git a/beets/autotag/__init__.py b/beets/autotag/__init__.py index feeefbf28..eabc41833 100644 --- a/beets/autotag/__init__.py +++ b/beets/autotag/__init__.py @@ -166,7 +166,38 @@ def correct_list_fields(m: LibModel) -> None: elif list_val: setattr(m, single_field, list_val[0]) + def migrate_legacy_genres() -> None: + """Migrate comma-separated genre strings to genres list. + + For users upgrading from previous versions, their genre field may + contain comma-separated values (e.g., "Rock, Alternative, Indie"). + This migration splits those values into the genres list on first access, + avoiding the need to reimport the entire library. + """ + genre_val = getattr(m, "genre", "") + genres_val = getattr(m, "genres", []) + + # Only migrate if genres list is empty and genre contains separators + if not genres_val and genre_val: + # Try common separators used by lastgenre and other tools + for separator in [", ", "; ", " / "]: + if separator in genre_val: + # Split and clean the genre string + split_genres = [ + g.strip() + for g in genre_val.split(separator) + if g.strip() + ] + if len(split_genres) > 1: + # Found a valid split - populate genres list + setattr(m, "genres", split_genres) + # Clear genre so ensure_first_value sets it correctly + setattr(m, "genre", "") + break + ensure_first_value("albumtype", "albumtypes") + migrate_legacy_genres() + ensure_first_value("genre", "genres") if hasattr(m, "mb_artistids"): ensure_first_value("mb_artistid", "mb_artistids") diff --git a/beets/autotag/hooks.py b/beets/autotag/hooks.py index 82e685b7a..7818d5f21 100644 --- a/beets/autotag/hooks.py +++ b/beets/autotag/hooks.py @@ -76,6 +76,7 @@ class Info(AttrDict[Any]): data_source: str | None = None, data_url: str | None = None, genre: str | None = None, + genres: list[str] | None = None, media: str | None = None, **kwargs, ) -> None: @@ -91,6 +92,7 @@ class Info(AttrDict[Any]): self.data_source = data_source self.data_url = data_url self.genre = genre + self.genres = genres or [] self.media = media self.update(kwargs) diff --git a/beets/library/models.py b/beets/library/models.py index e26f2ced3..1f01581c2 100644 --- a/beets/library/models.py +++ b/beets/library/models.py @@ -242,6 +242,7 @@ class Album(LibModel): "albumartists_credit": types.MULTI_VALUE_DSV, "album": types.STRING, "genre": types.STRING, + "genres": types.MULTI_VALUE_DSV, "style": types.STRING, "discogs_albumid": types.INTEGER, "discogs_artistid": types.INTEGER, @@ -298,6 +299,7 @@ class Album(LibModel): "albumartists_credit", "album", "genre", + "genres", "style", "discogs_albumid", "discogs_artistid", @@ -651,6 +653,7 @@ class Item(LibModel): "albumartist_credit": types.STRING, "albumartists_credit": types.MULTI_VALUE_DSV, "genre": types.STRING, + "genres": types.MULTI_VALUE_DSV, "style": types.STRING, "discogs_albumid": types.INTEGER, "discogs_artistid": types.INTEGER, diff --git a/beets/ui/commands/__init__.py b/beets/ui/commands/__init__.py index e1d0389a3..b4eebb53f 100644 --- a/beets/ui/commands/__init__.py +++ b/beets/ui/commands/__init__.py @@ -24,6 +24,7 @@ from .fields import fields_cmd from .help import HelpCommand from .import_ import import_cmd from .list import list_cmd +from .migrate import migrate_cmd from .modify import modify_cmd from .move import move_cmd from .remove import remove_cmd @@ -52,12 +53,13 @@ default_commands = [ HelpCommand(), import_cmd, list_cmd, - update_cmd, - remove_cmd, - stats_cmd, - version_cmd, + migrate_cmd, modify_cmd, move_cmd, + remove_cmd, + stats_cmd, + update_cmd, + version_cmd, write_cmd, config_cmd, completion_cmd, diff --git a/beets/ui/commands/migrate.py b/beets/ui/commands/migrate.py new file mode 100644 index 000000000..2cb7e0d59 --- /dev/null +++ b/beets/ui/commands/migrate.py @@ -0,0 +1,98 @@ +"""The 'migrate' command: migrate library data for format changes.""" + +from beets import logging, ui +from beets.autotag import correct_list_fields + +# Global logger. +log = logging.getLogger("beets") + + +def migrate_genres(lib, pretend=False): + """Migrate comma-separated genre strings to genres list. + + For users upgrading from previous versions, their genre field may + contain comma-separated values (e.g., "Rock, Alternative, Indie"). + This command splits those values into the genres list, avoiding + the need to reimport the entire library. + """ + items = lib.items() + migrated_count = 0 + total_items = 0 + + ui.print_("Scanning library for items with comma-separated genres...") + + for item in items: + total_items += 1 + genre_val = item.genre or "" + genres_val = item.genres or [] + + # Check if migration is needed + needs_migration = False + if not genres_val and genre_val: + for separator in [", ", "; ", " / "]: + if separator in genre_val: + split_genres = [ + g.strip() + for g in genre_val.split(separator) + if g.strip() + ] + if len(split_genres) > 1: + needs_migration = True + break + + if needs_migration: + migrated_count += 1 + old_genre = item.genre + old_genres = item.genres or [] + + if pretend: + # Just show what would change + ui.print_( + f" Would migrate: {item.artist} - {item.title}\n" + f" genre: {old_genre!r} -> {split_genres[0]!r}\n" + f" genres: {old_genres!r} -> {split_genres!r}" + ) + else: + # Actually migrate + correct_list_fields(item) + item.store() + log.debug( + "migrated: {} - {} ({} -> {})", + item.artist, + item.title, + old_genre, + item.genres, + ) + + # Show summary + if pretend: + ui.print_( + f"\nWould migrate {migrated_count} of {total_items} items " + f"(run without --pretend to apply changes)" + ) + else: + ui.print_( + f"\nMigrated {migrated_count} of {total_items} items with " + f"comma-separated genres" + ) + + +def migrate_func(lib, opts, args): + """Handle the migrate command.""" + if not args or args[0] == "genres": + migrate_genres(lib, pretend=opts.pretend) + else: + raise ui.UserError(f"unknown migration target: {args[0]}") + + +migrate_cmd = ui.Subcommand( + "migrate", help="migrate library data for format changes" +) +migrate_cmd.parser.add_option( + "-p", + "--pretend", + action="store_true", + help="show what would be changed without applying", +) +migrate_cmd.parser.usage = "%prog migrate genres [options]" +migrate_cmd.func = migrate_func diff --git a/beetsplug/beatport.py b/beetsplug/beatport.py index 718e0730e..8918a10cb 100644 --- a/beetsplug/beatport.py +++ b/beetsplug/beatport.py @@ -234,7 +234,9 @@ class BeatportObject: if "artists" in data: self.artists = [(x["id"], str(x["name"])) for x in data["artists"]] if "genres" in data: - self.genres = [str(x["name"]) for x in data["genres"]] + genre_list = [str(x["name"]) for x in data["genres"]] + # Remove duplicates while preserving order + self.genres = list(dict.fromkeys(genre_list)) def artists_str(self) -> str | None: if self.artists is not None: @@ -306,11 +308,16 @@ class BeatportTrack(BeatportObject): self.bpm = data.get("bpm") self.initial_key = str((data.get("key") or {}).get("shortName")) - # Use 'subgenre' and if not present, 'genre' as a fallback. + # Extract genres list from subGenres or genres if data.get("subGenres"): - self.genre = str(data["subGenres"][0].get("name")) + genre_list = [str(x.get("name")) for x in data["subGenres"]] elif data.get("genres"): - self.genre = str(data["genres"][0].get("name")) + genre_list = [str(x.get("name")) for x in data["genres"]] + else: + genre_list = [] + + # Remove duplicates while preserving order + self.genres = list(dict.fromkeys(genre_list)) class BeatportPlugin(MetadataSourcePlugin): @@ -484,6 +491,7 @@ class BeatportPlugin(MetadataSourcePlugin): data_source=self.data_source, data_url=release.url, genre=release.genre, + genres=release.genres, year=release_date.year if release_date else None, month=release_date.month if release_date else None, day=release_date.day if release_date else None, @@ -509,6 +517,7 @@ class BeatportPlugin(MetadataSourcePlugin): bpm=track.bpm, initial_key=track.initial_key, genre=track.genre, + genres=track.genres, ) def _get_artist(self, artists): diff --git a/beetsplug/lastgenre/__init__.py b/beetsplug/lastgenre/__init__.py index f7aef0261..a75af0aec 100644 --- a/beetsplug/lastgenre/__init__.py +++ b/beetsplug/lastgenre/__init__.py @@ -329,26 +329,23 @@ class LastGenrePlugin(plugins.BeetsPlugin): # Main processing: _get_genre() and helpers. - def _format_and_stringify(self, tags: list[str]) -> str: - """Format to title_case if configured and return as delimited string.""" + def _format_genres(self, tags: list[str]) -> list[str]: + """Format to title_case if configured and return as list.""" if self.config["title_case"]: - formatted = [tag.title() for tag in tags] + return [tag.title() for tag in tags] else: - formatted = tags - - return self.config["separator"].as_str().join(formatted) + return tags def _get_existing_genres(self, obj: LibModel) -> list[str]: """Return a list of genres for this Item or Album. Empty string genres are removed.""" - separator = self.config["separator"].get() if isinstance(obj, library.Item): - item_genre = obj.get("genre", with_album=False).split(separator) + genres_list = obj.get("genres", with_album=False) else: - item_genre = obj.get("genre").split(separator) + genres_list = obj.get("genres") # Filter out empty strings - return [g for g in item_genre if g] + return [g for g in genres_list if g] if genres_list else [] def _combine_resolve_and_log( self, old: list[str], new: list[str] @@ -359,8 +356,8 @@ class LastGenrePlugin(plugins.BeetsPlugin): combined = old + new return self._resolve_genres(combined) - def _get_genre(self, obj: LibModel) -> tuple[str | None, ...]: - """Get the final genre string for an Album or Item object. + def _get_genre(self, obj: LibModel) -> tuple[list[str], str]: + """Get the final genre list for an Album or Item object. `self.sources` specifies allowed genre sources. Starting with the first source in this tuple, the following stages run through until a genre is @@ -370,9 +367,9 @@ class LastGenrePlugin(plugins.BeetsPlugin): - artist, albumartist or "most popular track genre" (for VA-albums) - original fallback - configured fallback - - None + - empty list - A `(genre, label)` pair is returned, where `label` is a string used for + A `(genres, label)` pair is returned, where `label` is a string used for logging. For example, "keep + artist, whitelist" indicates that existing genres were combined with new last.fm genres and whitelist filtering was applied, while "artist, any" means only new last.fm genres are included @@ -391,7 +388,7 @@ class LastGenrePlugin(plugins.BeetsPlugin): label = f"{stage_label}, {suffix}" if keep_genres: label = f"keep + {label}" - return self._format_and_stringify(resolved_genres), label + return self._format_genres(resolved_genres), label return None keep_genres = [] @@ -400,10 +397,7 @@ class LastGenrePlugin(plugins.BeetsPlugin): if genres and not self.config["force"]: # Without force pre-populated tags are returned as-is. - label = "keep any, no-force" - if isinstance(obj, library.Item): - return obj.get("genre", with_album=False), label - return obj.get("genre"), label + return genres, "keep any, no-force" if self.config["force"]: # Force doesn't keep any unless keep_existing is set. @@ -480,8 +474,14 @@ class LastGenrePlugin(plugins.BeetsPlugin): # Nothing found, leave original if configured and valid. if obj.genre and self.config["keep_existing"]: - if not self.whitelist or self._is_valid(obj.genre.lower()): - return obj.genre, "original fallback" + # Check if at least one genre is valid + valid_genres = [ + g + for g in genres + if not self.whitelist or self._is_valid(g.lower()) + ] + if valid_genres: + return valid_genres, "original fallback" else: # If the original genre doesn't match a whitelisted genre, check # if we can canonicalize it to find a matching, whitelisted genre! @@ -490,22 +490,23 @@ class LastGenrePlugin(plugins.BeetsPlugin): ): return result - # Return fallback string. + # Return fallback as a list. if fallback := self.config["fallback"].get(): - return fallback, "fallback" + return [fallback], "fallback" # No fallback configured. - return None, "fallback unconfigured" + return [], "fallback unconfigured" # Beets plugin hooks and CLI. def _fetch_and_log_genre(self, obj: LibModel) -> None: """Fetch genre and log it.""" self._log.info(str(obj)) - obj.genre, label = self._get_genre(obj) - self._log.debug("Resolved ({}): {}", label, obj.genre) + genres_list, label = self._get_genre(obj) + obj.genres = genres_list + self._log.debug("Resolved ({}): {}", label, genres_list) - ui.show_model_changes(obj, fields=["genre"], print_obj=False) + ui.show_model_changes(obj, fields=["genres"], print_obj=False) @singledispatchmethod def _process(self, obj: LibModel, write: bool) -> None: diff --git a/beetsplug/musicbrainz.py b/beetsplug/musicbrainz.py index ffef366ae..695ce8790 100644 --- a/beetsplug/musicbrainz.py +++ b/beetsplug/musicbrainz.py @@ -644,10 +644,11 @@ class MusicBrainzPlugin(MusicBrainzAPIMixin, MetadataSourcePlugin): for source in sources: for genreitem in source: genres[genreitem["name"]] += int(genreitem["count"]) - info.genre = "; ".join( + genre_list = [ genre for genre, _count in sorted(genres.items(), key=lambda g: -g[1]) - ) + ] + info.genres = genre_list # We might find links to external sources (Discogs, Bandcamp, ...) external_ids = self.config["external_ids"].get() diff --git a/docs/changelog.rst b/docs/changelog.rst index b15eb2944..f714fc766 100644 --- a/docs/changelog.rst +++ b/docs/changelog.rst @@ -9,9 +9,24 @@ below! Unreleased ---------- -.. - New features - ~~~~~~~~~~~~ +New features +~~~~~~~~~~~~ + +- Add native support for multiple genres per album/track. The ``genres`` field + now stores genres as a list and is written to files as multiple individual + genre tags (e.g., separate GENRE tags for FLAC/MP3). The single ``genre`` + field is automatically synchronized to contain the first genre from the list + for backward compatibility. The :doc:`plugins/musicbrainz`, + :doc:`plugins/beatport`, and :doc:`plugins/lastgenre` plugins have been + updated to populate the ``genres`` field as a list. + + **Migration**: Existing libraries with comma-separated, semicolon-separated, + or slash-separated genre strings (e.g., ``"Rock, Alternative, Indie"``) will + be automatically migrated to the ``genres`` list when items are accessed. No + manual reimport or ``mbsync`` is required. For users who prefer explicit + control, a new ``beet migrate genres`` command is available to migrate the + entire library at once. Use ``beet migrate genres --pretend`` to preview + changes before applying them. Bug fixes ~~~~~~~~~ diff --git a/test/plugins/test_beatport.py b/test/plugins/test_beatport.py index b92a3bf15..dbfb89c9c 100644 --- a/test/plugins/test_beatport.py +++ b/test/plugins/test_beatport.py @@ -583,7 +583,8 @@ class BeatportTest(BeetsTestCase): def test_genre_applied(self): for track, test_track in zip(self.tracks, self.test_tracks): - assert track.genre == test_track.genre + # BeatportTrack now has genres as a list + assert track.genres == [test_track.genre] class BeatportResponseEmptyTest(unittest.TestCase): @@ -634,7 +635,8 @@ class BeatportResponseEmptyTest(unittest.TestCase): self.test_tracks[0]["subGenres"] = [] - assert tracks[0].genre == self.test_tracks[0]["genres"][0]["name"] + # BeatportTrack now has genres as a list + assert tracks[0].genres == [self.test_tracks[0]["genres"][0]["name"]] def test_genre_empty(self): """No 'genre' is provided. Test if 'sub_genre' is applied.""" @@ -643,4 +645,5 @@ class BeatportResponseEmptyTest(unittest.TestCase): self.test_tracks[0]["genres"] = [] - assert tracks[0].genre == self.test_tracks[0]["subGenres"][0]["name"] + # BeatportTrack now has genres as a list + assert tracks[0].genres == [self.test_tracks[0]["subGenres"][0]["name"]] diff --git a/test/plugins/test_lastgenre.py b/test/plugins/test_lastgenre.py index f499992c6..e95173115 100644 --- a/test/plugins/test_lastgenre.py +++ b/test/plugins/test_lastgenre.py @@ -80,13 +80,15 @@ class LastGenrePluginTest(IOMixin, PluginTestCase): self._setup_config(canonical="", whitelist={"rock"}) assert self.plugin._resolve_genres(["delta blues"]) == [] - def test_format_and_stringify(self): - """Format genres list and return them as a separator-delimited string.""" + def test_format_genres(self): + """Format genres list with title case if configured.""" self._setup_config(count=2) - assert ( - self.plugin._format_and_stringify(["jazz", "pop", "rock", "blues"]) - == "Jazz, Pop, Rock, Blues" - ) + assert self.plugin._format_genres(["jazz", "pop", "rock", "blues"]) == [ + "Jazz", + "Pop", + "Rock", + "Blues", + ] def test_count_c14n(self): """Keep the n first genres, after having applied c14n when necessary""" @@ -144,7 +146,7 @@ class LastGenrePluginTest(IOMixin, PluginTestCase): albumartist="Pretend Artist", artist="Pretend Artist", title="Pretend Track", - genre="Original Genre", + genres=["Original Genre"], ) album = self.lib.add_album([item]) @@ -155,10 +157,10 @@ class LastGenrePluginTest(IOMixin, PluginTestCase): with patch("beetsplug.lastgenre.Item.store", unexpected_store): output = self.run_with_output("lastgenre", "--pretend") - assert "Mock Genre" in output + assert "genres:" in output album.load() - assert album.genre == "Original Genre" - assert album.items()[0].genre == "Original Genre" + assert album.genres == ["Original Genre"] + assert album.items()[0].genres == ["Original Genre"] def test_no_duplicate(self): """Remove duplicated genres.""" @@ -219,7 +221,7 @@ class LastGenrePluginTest(IOMixin, PluginTestCase): { "album": ["Jazz"], }, - ("Blues, Jazz", "keep + album, whitelist"), + (["Blues", "Jazz"], "keep + album, whitelist"), ), # 1 - force and keep whitelisted, unknown original ( @@ -235,7 +237,7 @@ class LastGenrePluginTest(IOMixin, PluginTestCase): { "album": ["Jazz"], }, - ("Blues, Jazz", "keep + album, whitelist"), + (["Blues", "Jazz"], "keep + album, whitelist"), ), # 2 - force and keep whitelisted on empty tag ( @@ -251,7 +253,7 @@ class LastGenrePluginTest(IOMixin, PluginTestCase): { "album": ["Jazz"], }, - ("Jazz", "album, whitelist"), + (["Jazz"], "album, whitelist"), ), # 3 force and keep, artist configured ( @@ -268,7 +270,7 @@ class LastGenrePluginTest(IOMixin, PluginTestCase): "album": ["Jazz"], "artist": ["Pop"], }, - ("Blues, Pop", "keep + artist, whitelist"), + (["Blues", "Pop"], "keep + artist, whitelist"), ), # 4 - don't force, disabled whitelist ( @@ -284,7 +286,7 @@ class LastGenrePluginTest(IOMixin, PluginTestCase): { "album": ["Jazz"], }, - ("any genre", "keep any, no-force"), + (["any genre"], "keep any, no-force"), ), # 5 - don't force and empty is regular last.fm fetch; no whitelist too ( @@ -300,7 +302,7 @@ class LastGenrePluginTest(IOMixin, PluginTestCase): { "album": ["Jazzin"], }, - ("Jazzin", "album, any"), + (["Jazzin"], "album, any"), ), # 6 - fallback to next stages until found ( @@ -318,7 +320,7 @@ class LastGenrePluginTest(IOMixin, PluginTestCase): "album": None, "artist": ["Jazz"], }, - ("Unknown Genre, Jazz", "keep + artist, any"), + (["Unknown Genre", "Jazz"], "keep + artist, any"), ), # 7 - Keep the original genre when force and keep_existing are on, and # whitelist is disabled @@ -338,7 +340,7 @@ class LastGenrePluginTest(IOMixin, PluginTestCase): "album": None, "artist": None, }, - ("any existing", "original fallback"), + (["any existing"], "original fallback"), ), # 7.1 - Keep the original genre when force and keep_existing are on, and # whitelist is enabled, and genre is valid. @@ -358,7 +360,7 @@ class LastGenrePluginTest(IOMixin, PluginTestCase): "album": None, "artist": None, }, - ("Jazz", "original fallback"), + (["Jazz"], "original fallback"), ), # 7.2 - Return the configured fallback when force is on but # keep_existing is not. @@ -378,7 +380,7 @@ class LastGenrePluginTest(IOMixin, PluginTestCase): "album": None, "artist": None, }, - ("fallback genre", "fallback"), + (["fallback genre"], "fallback"), ), # 8 - fallback to fallback if no original ( @@ -397,7 +399,7 @@ class LastGenrePluginTest(IOMixin, PluginTestCase): "album": None, "artist": None, }, - ("fallback genre", "fallback"), + (["fallback genre"], "fallback"), ), # 9 - null charachter as separator ( @@ -409,12 +411,13 @@ class LastGenrePluginTest(IOMixin, PluginTestCase): "separator": "\u0000", "canonical": False, "prefer_specific": False, + "count": 10, }, "Blues", { "album": ["Jazz"], }, - ("Blues\u0000Jazz", "keep + album, whitelist"), + (["Blues", "Jazz"], "keep + album, whitelist"), ), # 10 - limit a lot of results ( @@ -432,7 +435,10 @@ class LastGenrePluginTest(IOMixin, PluginTestCase): { "album": ["Jazz", "Bebop", "Hardbop"], }, - ("Blues, Rock, Metal, Jazz, Bebop", "keep + album, whitelist"), + ( + ["Blues", "Rock", "Metal", "Jazz", "Bebop"], + "keep + album, whitelist", + ), ), # 11 - force off does not rely on configured separator ( @@ -448,7 +454,7 @@ class LastGenrePluginTest(IOMixin, PluginTestCase): { "album": ["Jazz", "Bebop"], }, - ("not ; configured | separator", "keep any, no-force"), + (["not ; configured | separator"], "keep any, no-force"), ), # 12 - fallback to next stage (artist) if no allowed original present # and no album genre were fetched. @@ -468,7 +474,7 @@ class LastGenrePluginTest(IOMixin, PluginTestCase): "album": None, "artist": ["Jazz"], }, - ("Jazz", "keep + artist, whitelist"), + (["Jazz"], "keep + artist, whitelist"), ), # 13 - canonicalization transforms non-whitelisted genres to canonical forms # @@ -488,7 +494,7 @@ class LastGenrePluginTest(IOMixin, PluginTestCase): { "album": ["acid techno"], }, - ("Techno, Electronic", "album, whitelist"), + (["Techno", "Electronic"], "album, whitelist"), ), # 14 - canonicalization transforms whitelisted genres to canonical forms and # includes originals @@ -512,7 +518,13 @@ class LastGenrePluginTest(IOMixin, PluginTestCase): "album": ["acid house"], }, ( - "Detroit Techno, Techno, Electronic, Acid House, House", + [ + "Detroit Techno", + "Techno", + "Electronic", + "Acid House", + "House", + ], "keep + album, whitelist", ), ), @@ -537,7 +549,7 @@ class LastGenrePluginTest(IOMixin, PluginTestCase): "album": ["Detroit Techno"], }, ( - "Disco, Electronic, Detroit Techno, Techno", + ["Disco", "Electronic", "Detroit Techno", "Techno"], "keep + album, whitelist", ), ), @@ -556,13 +568,13 @@ class LastGenrePluginTest(IOMixin, PluginTestCase): "prefer_specific": False, "count": 10, }, - "Cosmic Disco", + ["Cosmic Disco"], { "album": [], "artist": [], }, ( - "Disco, Electronic", + ["Disco", "Electronic"], "keep + original fallback, whitelist", ), ), @@ -592,9 +604,31 @@ def test_get_genre(config_values, item_genre, mock_genres, expected_result): # Configure plugin.config.set(config_values) plugin.setup() # Loads default whitelist and canonicalization tree + item = _common.item() - item.genre = item_genre + # Set genres as a list - if item_genre is a string, convert it to list + if item_genre: + # For compatibility with old separator-based tests, split if needed + if ( + "separator" in config_values + and config_values["separator"] in item_genre + ): + sep = config_values["separator"] + item.genres = [ + g.strip() for g in item_genre.split(sep) if g.strip() + ] + else: + # Assume comma-separated if no specific separator + if ", " in item_genre: + item.genres = [ + g.strip() for g in item_genre.split(", ") if g.strip() + ] + else: + item.genres = [item_genre] + else: + item.genres = [] # Run res = plugin._get_genre(item) - assert res == expected_result + expected_genres, expected_label = expected_result + assert res == (expected_genres, expected_label) diff --git a/test/plugins/test_musicbrainz.py b/test/plugins/test_musicbrainz.py index 8d7c5a2f8..4ebce1b01 100644 --- a/test/plugins/test_musicbrainz.py +++ b/test/plugins/test_musicbrainz.py @@ -526,14 +526,14 @@ class MBAlbumInfoTest(MusicBrainzTestCase): config["musicbrainz"]["genres_tag"] = "genre" release = self._make_release() d = self.mb.album_info(release) - assert d.genre == "GENRE" + assert d.genres == ["GENRE"] def test_tags(self): config["musicbrainz"]["genres"] = True config["musicbrainz"]["genres_tag"] = "tag" release = self._make_release() d = self.mb.album_info(release) - assert d.genre == "TAG" + assert d.genres == ["TAG"] def test_no_genres(self): config["musicbrainz"]["genres"] = False diff --git a/test/test_autotag.py b/test/test_autotag.py index 119ca15e8..e094d0ddc 100644 --- a/test/test_autotag.py +++ b/test/test_autotag.py @@ -475,3 +475,116 @@ def test_correct_list_fields( single_val, list_val = item[single_field], item[list_field] assert (not single_val and not list_val) or single_val == list_val[0] + + +# Tests for multi-value genres functionality +class TestGenreSync: + """Test the genre/genres field synchronization.""" + + def test_genres_list_to_genre_first(self): + """Genres list sets genre to first item.""" + item = Item(genres=["Rock", "Alternative", "Indie"]) + correct_list_fields(item) + + assert item.genre == "Rock" + assert item.genres == ["Rock", "Alternative", "Indie"] + + def test_genre_string_to_genres_list(self): + """Genre string becomes first item in genres list.""" + item = Item(genre="Rock") + correct_list_fields(item) + + assert item.genre == "Rock" + assert item.genres == ["Rock"] + + def test_genre_and_genres_both_present(self): + """When both genre and genres exist, genre becomes first in list.""" + item = Item(genre="Jazz", genres=["Rock", "Alternative"]) + correct_list_fields(item) + + # genre should be prepended to genres list (deduplicated) + assert item.genre == "Jazz" + assert item.genres == ["Jazz", "Rock", "Alternative"] + + def test_empty_genre(self): + """Empty genre field.""" + item = Item(genre="") + correct_list_fields(item) + + assert item.genre == "" + assert item.genres == [] + + def test_empty_genres(self): + """Empty genres list.""" + item = Item(genres=[]) + correct_list_fields(item) + + assert item.genre == "" + assert item.genres == [] + + def test_none_values(self): + """Handle None values in genre/genres fields without errors.""" + # Test with None genre + item = Item(genre=None, genres=["Rock"]) + correct_list_fields(item) + assert item.genres == ["Rock"] + assert item.genre == "Rock" + + # Test with None genres + item = Item(genre="Jazz", genres=None) + correct_list_fields(item) + assert item.genre == "Jazz" + assert item.genres == ["Jazz"] + + def test_none_both(self): + """Handle None in both genre and genres.""" + item = Item(genre=None, genres=None) + correct_list_fields(item) + + assert item.genres == [] + assert item.genre == "" + + def test_migrate_comma_separated_genres(self): + """Migrate legacy comma-separated genre strings.""" + item = Item(genre="Rock, Alternative, Indie", genres=[]) + correct_list_fields(item) + + # Should split into genres list + assert item.genres == ["Rock", "Alternative", "Indie"] + # Genre becomes first item after migration + assert item.genre == "Rock" + + def test_migrate_semicolon_separated_genres(self): + """Migrate legacy semicolon-separated genre strings.""" + item = Item(genre="Rock; Alternative; Indie", genres=[]) + correct_list_fields(item) + + assert item.genres == ["Rock", "Alternative", "Indie"] + assert item.genre == "Rock" + + def test_migrate_slash_separated_genres(self): + """Migrate legacy slash-separated genre strings.""" + item = Item(genre="Rock / Alternative / Indie", genres=[]) + correct_list_fields(item) + + assert item.genres == ["Rock", "Alternative", "Indie"] + assert item.genre == "Rock" + + def test_no_migration_when_genres_exists(self): + """Don't migrate if genres list already populated.""" + item = Item(genre="Jazz, Blues", genres=["Rock", "Pop"]) + correct_list_fields(item) + + # Existing genres list should be preserved + # The full genre string is prepended (migration doesn't run when genres exists) + assert item.genres == ["Jazz, Blues", "Rock", "Pop"] + assert item.genre == "Jazz, Blues" + + def test_no_migration_single_genre(self): + """Don't split single genres without separators.""" + item = Item(genre="Rock", genres=[]) + correct_list_fields(item) + + # Single genre (no separator) should not trigger migration + assert item.genres == ["Rock"] + assert item.genre == "Rock" diff --git a/test/test_library.py b/test/test_library.py index 4acf34746..a0eb9fd9a 100644 --- a/test/test_library.py +++ b/test/test_library.py @@ -688,19 +688,47 @@ class DestinationFunctionTest(BeetsTestCase, PathFormattingMixin): self._assert_dest(b"/base/not_played") def test_first(self): - self.i.genres = "Pop; Rock; Classical Crossover" - self._setf("%first{$genres}") + self.i.genre = "Pop; Rock; Classical Crossover" + self._setf("%first{$genre}") self._assert_dest(b"/base/Pop") def test_first_skip(self): - self.i.genres = "Pop; Rock; Classical Crossover" - self._setf("%first{$genres,1,2}") + self.i.genre = "Pop; Rock; Classical Crossover" + self._setf("%first{$genre,1,2}") self._assert_dest(b"/base/Classical Crossover") def test_first_different_sep(self): self._setf("%first{Alice / Bob / Eve,2,0, / , & }") self._assert_dest(b"/base/Alice & Bob") + def test_first_genres_list(self): + # Test that setting genres as a list syncs to genre field properly + # and works with %first template function + from beets.autotag import correct_list_fields + + # Clear the default genre first + self.i.genre = "" + self.i.genres = ["Pop", "Rock", "Classical Crossover"] + correct_list_fields(self.i) + # genre field should now be synced to first item + assert self.i.genre == "Pop" + # %first should work on the genre field + self._setf("%first{$genre}") + self._assert_dest(b"/base/Pop") + + def test_first_genres_list_skip(self): + # Test that the genres list is accessible as a multi-value field + from beets.autotag import correct_list_fields + + # Clear the default genre first + self.i.genre = "" + self.i.genres = ["Pop", "Rock", "Classical Crossover"] + correct_list_fields(self.i) + # Access the second genre directly using index (genres is a list) + # The genres field should be available as a multi-value field + assert self.i.genres[1] == "Rock" + assert len(self.i.genres) == 3 + class DisambiguationTest(BeetsTestCase, PathFormattingMixin): def setUp(self):