Closed
Conversation
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Hey there - I've reviewed your changes - here's some feedback:
- Extract the genre separator into a shared constant to avoid hardcoding '; ' across multiple modules and simplify future adjustments.
- Refactor the Beatport and MusicBrainz plugins to leverage the core split/join logic instead of duplicating it, ensuring consistent behavior.
- Consider adding a library load hook to backfill existing "genres" fields from current delimited "genre" values so users see the list immediately without reimporting metadata.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- Extract the genre separator into a shared constant to avoid hardcoding '; ' across multiple modules and simplify future adjustments.
- Refactor the Beatport and MusicBrainz plugins to leverage the core split/join logic instead of duplicating it, ensuring consistent behavior.
- Consider adding a library load hook to backfill existing "genres" fields from current delimited "genre" values so users see the list immediately without reimporting metadata.
## Individual Comments
### Comment 1
<location> `beetsplug/beatport.py:236-237` </location>
<code_context>
if "artists" in data:
self.artists = [(x["id"], str(x["name"])) for x in data["artists"]]
- if "genres" in data:
+ if "genres" in data and beets.config["multi_value_genres"]:
self.genres = [str(x["name"]) for x in data["genres"]]
</code_context>
<issue_to_address>
**suggestion (bug_risk):** The conditional assignment of self.genres may lead to missing genre data if multi_value_genres is disabled.
If multi_value_genres is disabled, self.genres remains unset even when genre data is available, which may cause issues elsewhere in the code. To ensure consistency, set self.genres to a single-element or empty list regardless of the config setting.
```suggestion
if "genres" in data:
if beets.config["multi_value_genres"]:
self.genres = [str(x["name"]) for x in data["genres"]]
else:
self.genres = [str(data["genres"][0]["name"])] if data["genres"] else []
```
</issue_to_address>
### Comment 2
<location> `beetsplug/beatport.py:318-320` </location>
<code_context>
+ else:
+ genre_list = []
+
+ if beets.config["multi_value_genres"]:
+ # New behavior: populate both genres list and joined string
+ self.genres = genre_list
+ self.genre = "; ".join(genre_list) if genre_list else None
+ else:
+ # Old behavior: only populate single genre field with first value
+ self.genre = genre_list[0] if genre_list else None
</code_context>
<issue_to_address>
**suggestion:** The genre string join uses a hardcoded separator; consider using a configurable separator.
Using a hardcoded '; ' separator may lead to inconsistency with user preferences. Adopting a configurable separator would align with other plugins and improve flexibility.
```suggestion
# New behavior: populate both genres list and joined string
self.genres = genre_list
separator = beets.config["genre_separator"].get(str, "; ")
self.genre = separator.join(genre_list) if genre_list else None
```
</issue_to_address>
### Comment 3
<location> `beetsplug/musicbrainz.py:744-750` </location>
<code_context>
+ genre_val = getattr(m, "genre")
+ genres_val = getattr(m, "genres")
+
+ if config["multi_value_genres"]:
+ # New behavior: sync all genres
+ # Standard separator used by MusicBrainz and other plugins
</code_context>
<issue_to_address>
**suggestion:** The genre string join uses a hardcoded separator; consider using a configurable separator.
Using a hardcoded '; ' separator may not align with user settings or other plugins. Please use the configurable separator from the user configuration.
</issue_to_address>
### Comment 4
<location> `test/test_autotag.py:523-531` </location>
<code_context>
+ assert item.genre == "Rock"
+ assert item.genres == ["Rock"]
+
+ def test_sync_genres_enabled_empty_genre(self):
+ """Empty genre field with multi_value_genres enabled."""
+ config["multi_value_genres"] = True
+
+ item = Item(genre="")
+ correct_list_fields(item)
+
+ assert item.genre == ""
+ assert item.genres == []
+
+ def test_sync_genres_enabled_empty_genres(self):
</code_context>
<issue_to_address>
**suggestion (testing):** Suggestion to add test for None values in genre/genres fields.
Add a test where genre or genres is set to None to ensure the sync logic handles these cases without errors.
</issue_to_address>
### Comment 5
<location> `test/test_autotag.py:503-491` </location>
<code_context>
+ assert item.genre == "Rock; Alternative; Indie"
+ assert item.genres == ["Rock", "Alternative", "Indie"]
+
+ def test_sync_genres_disabled_only_first(self):
+ """When multi_value_genres is disabled, only first genre is used."""
+ config["multi_value_genres"] = False
+
+ item = Item(genres=["Rock", "Alternative", "Indie"])
+ correct_list_fields(item)
+
+ assert item.genre == "Rock"
+ assert item.genres == ["Rock", "Alternative", "Indie"]
+
+ def test_sync_genres_disabled_string_becomes_list(self):
</code_context>
<issue_to_address>
**suggestion (testing):** Suggestion to add test for disabled config with empty genres list.
Please add a test for when multi_value_genres is False and genres is an empty list, to verify correct handling of genre in this scenario.
</issue_to_address>
### Comment 6
<location> `test/test_library.py:691` </location>
<code_context>
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")
</code_context>
<issue_to_address>
**question (testing):** Question about test coverage for new genres field in library tests.
Consider adding tests that specifically cover the new genres list field in library operations, such as queries or template rendering.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
| self._assert_dest(b"/base/not_played") | ||
|
|
||
| def test_first(self): | ||
| self.i.genres = "Pop; Rock; Classical Crossover" |
Contributor
There was a problem hiding this comment.
question (testing): Question about test coverage for new genres field in library tests.
Consider adding tests that specifically cover the new genres list field in library operations, such as queries or template rendering.
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## master #6168 +/- ##
==========================================
- Coverage 67.44% 67.42% -0.02%
==========================================
Files 136 136
Lines 18526 18571 +45
Branches 3129 3143 +14
==========================================
+ Hits 12494 12521 +27
- Misses 5369 5378 +9
- Partials 663 672 +9
🚀 New features to boost your workflow:
|
dunkla
force-pushed
the
claude/add-multiple-artists-01AKN5cZkyhLLwf2zh3ue8rT
branch
from
7d3f6b8 to
f36ea7f
Compare
Implements native multi-value genre support following the same pattern as multi-value artists. Adds a 'genres' field that stores genres as a list and writes them as multiple individual genre tags to files. Features: - New 'genres' field (MULTI_VALUE_DSV) for albums and tracks - Bidirectional sync between 'genre' (string) and 'genres' (list) - Config option 'multi_value_genres' (default: yes) to enable/disable - Config option 'genre_separator' (default: ', ') for joining genres into the single 'genre' field - matches lastgenre's default separator - Updated MusicBrainz, Beatport, and LastGenre plugins to populate 'genres' field - LastGenre plugin now uses global genre_separator when multi_value_genres is enabled for consistency - Comprehensive test coverage (10 tests for sync logic) - Full documentation in changelog and reference/config.rst Backward Compatibility: - When multi_value_genres=yes: 'genre' field maintained as joined string for backward compatibility, 'genres' is the authoritative list - When multi_value_genres=no: Preserves old behavior (only first genre) - Default separator matches lastgenre's default for seamless migration Migration: - Most users (using lastgenre's default) need no configuration changes - Users with custom lastgenre separator should set genre_separator to match their existing data - Users can opt-out entirely with multi_value_genres: no Code Review Feedback Addressed: - Extracted genre separator into configurable option (not hardcoded) - Fixed Beatport plugin to always populate genres field consistently - Added tests for None values and edge cases - Handle None values gracefully in sync logic - Added migration documentation for smooth user experience - Made separator user-configurable instead of constant - Changed default to ', ' for seamless migration (matches lastgenre)
dunkla
force-pushed
the
claude/add-multiple-artists-01AKN5cZkyhLLwf2zh3ue8rT
branch
from
30b978a to
11f38e8
Compare
dunkla
deleted the
claude/add-multiple-artists-01AKN5cZkyhLLwf2zh3ue8rT
branch
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
2 participants
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.
Add native support for multiple genres per album/track
Background
This PR addresses a long-standing feature request to add native support for multiple genres, similar to how beets already handles multiple artists. There was a previous attempt in PR #4751 that stalled. This implementation takes a fresh approach while incorporating feedback from that discussion.
Problem
Currently, beets only supports a single
genrefield (STRING type). Many users work around this by storing multiple genres as a delimited string (e.g.,"Rock; Alternative; Indie"), but this is not a native list field and doesn't write properly to files as separate genre tags.Meanwhile, beets has had native multi-value support for
artists,albumartists, and other fields for years, and the underlying MediaFile library has supported multiple genres since 2014 (beetbox/mediafile#527).Solution
This PR adds a
genresfield (MULTI_VALUE_DSV type) that stores genres as a proper list and writes them to files as individual genre tags (e.g., separate GENRE tags for FLAC/MP3 files).Key features:
genresfield: Stores genres as a list using null-delimited string format in the databasegenrefield remains and stores a joined string (e.g.,"Rock; Alternative; Indie") for users who rely on itgenre↔genresfields are kept in sync automatically viacorrect_list_fields()multi_value_genresconfig option (default:yes) controls whether to use the new multi-value behavior or fall back to the old single-genre behaviorgenreslistImplementation Details
Database schema (
beets/library/models.py):genres: types.MULTI_VALUE_DSVto bothAlbumandItemmodelsitem_keysfor proper synchronizationSynchronization logic (
beets/autotag/__init__.py):sync_genre_fields()function incorrect_list_fields()multi_value_genres=true:genreslist is authoritative,genreis the joined stringmulti_value_genres=false: Falls back to old behavior (only first genre)Plugin support:
beetsplug/musicbrainz.py): Populatesgenreslist from tag databeetsplug/beatport.py): Extracts multiple genres and subgenresbeetsplug/lastgenre/__init__.py): Updated to checkgenreslist first, fallback to splittinggenrefieldConfiguration (
beets/config_default.yaml):multi_value_genres: yes(enabled by default)Addresses Previous Feedback
This PR incorporates feedback from the previous attempt (#4751):
✅ Changelog entry added
✅ Config option to make behavior optional
✅ LastGenre plugin compatibility maintained
✅ Clean, single squashed commit
✅ Rebased on current master
Testing
Added comprehensive test coverage:
test/test_autotag.py::TestGenreSynccovering all sync scenariosMigration Path
No database migration needed. Existing installations will:
genrefield values intactgenreslist on next metadata updatemulti_value_genres: noin configExample Usage
After import/update, items will have:
File format support (via MediaFile):
This brings genre handling in line with how beets already handles artists, providing a cleaner and more consistent experience for users managing multi-genre music libraries.
And: yes, this is written with heavy support from claude.