Add native support for multiple genres per album/track #6169
Conversation
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
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)
![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 and they look great!
Prompt for AI Agents
Please address the comments from this code review:
## Individual Comments
### Comment 1
<location> `beetsplug/beatport.py:236-242` </location>
<code_context>
if "genres" in data:
- self.genres = [str(x["name"]) for x in data["genres"]]
+ genre_list = [str(x["name"]) for x in data["genres"]]
+ if beets.config["multi_value_genres"]:
+ self.genres = genre_list
+ else:
+ # Even when disabled, populate with first genre for consistency
</code_context>
<issue_to_address>
**suggestion (bug_risk):** Populating genres with all values may introduce duplicates if Beatport returns repeated genre names.
Consider removing duplicate genre names from genre_list before assigning it to self.genres.
```suggestion
if "genres" in data:
genre_list = [str(x["name"]) for x in data["genres"]]
# Remove duplicates while preserving order
genre_list = list(dict.fromkeys(genre_list))
if beets.config["multi_value_genres"]:
self.genres = genre_list
else:
# Even when disabled, populate with first genre for consistency
self.genres = [genre_list[0]] if genre_list else []
```
</issue_to_address>
### Comment 2
<location> `test/test_autotag.py:554-563` </location>
<code_context>
+ assert item.genres == ["Rock", "Alternative", "Indie"]
+ assert item.genre == "Rock, Alternative, Indie"
+
+ def test_sync_genres_priority_list_over_string(self):
+ """When both genre and genres exist, genres list takes priority."""
+ config["multi_value_genres"] = True
+
+ item = Item(genre="Jazz", genres=["Rock", "Alternative"])
+ correct_list_fields(item)
+
+ # genres list should take priority and update genre string
+ assert item.genres == ["Rock", "Alternative"]
+ assert item.genre == "Rock, Alternative"
+
+ def test_sync_genres_none_values(self):
</code_context>
<issue_to_address>
**suggestion (testing):** Consider testing the scenario where both genre and genres are set but have conflicting values and multi_value_genres is disabled.
Please add a test for the case where multi_value_genres is False and genre and genres have different values, to verify which field takes precedence and how synchronization is handled.
</issue_to_address>
### Comment 3
<location> `test/test_autotag.py:581-590` </location>
<code_context>
+ assert item.genre == "Jazz"
+ assert item.genres == ["Jazz"]
+
+ def test_sync_genres_disabled_empty_genres(self):
+ """Handle disabled config with empty genres list."""
+ config["multi_value_genres"] = False
+
+ item = Item(genres=[])
+ correct_list_fields(item)
+
+ # Should handle empty list without errors
+ assert item.genres == []
+ assert item.genre == ""
</code_context>
<issue_to_address>
**suggestion (testing):** Consider adding a test for genres=None when multi_value_genres is False.
Testing genres=None with multi_value_genres=False will help verify correct fallback behavior and error handling.
```suggestion
def test_sync_genres_disabled_empty_genres(self):
"""Handle disabled config with empty genres list."""
config["multi_value_genres"] = False
item = Item(genres=[])
correct_list_fields(item)
# Should handle empty list without errors
assert item.genres == []
assert item.genre == ""
def test_sync_genres_disabled_none_genres(self):
"""Handle disabled config with genres=None."""
config["multi_value_genres"] = False
item = Item(genres=None)
correct_list_fields(item)
# Should handle None without errors
assert item.genres == []
assert item.genre == ""
```
</issue_to_address>
### Comment 4
<location> `test/test_library.py:690-693` </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>
**suggestion (testing):** Tests in test_library.py should be updated to cover the new genres field.
Add tests that assign genres as a list and check template functions like %first to ensure correct handling of the new field.
```suggestion
def test_first(self):
self.i.genre = "Pop; Rock; Classical Crossover"
self._setf("%first{$genre}")
self._assert_dest(b"/base/Pop")
def test_first_genres_list(self):
self.i.genres = ["Pop", "Rock", "Classical Crossover"]
self._setf("%first{$genres}")
self._assert_dest(b"/base/Pop")
def test_first_genres_list_skip(self):
self.i.genres = ["Pop", "Rock", "Classical Crossover"]
self._setf("%first{$genres,1,2}")
self._assert_dest(b"/base/Classical Crossover")
```
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## master #6169 +/- ##
==========================================
- Coverage 67.44% 67.41% -0.03%
==========================================
Files 136 136
Lines 18526 18578 +52
Branches 3129 3145 +16
==========================================
+ Hits 12494 12524 +30
- Misses 5369 5380 +11
- Partials 663 674 +11
🚀 New features to boost your workflow:
|
- Add duplicate removal in Beatport plugin using dict.fromkeys() - Add test for conflicting genre/genres values when disabled - Add test for handling genres=None when feature is disabled - Add library tests for genres field with template functions - Fix lastgenre test case to include count parameter - Make lastgenre plugin use global genre_separator for consistency All changes ensure plugins use consistent separators and handle edge cases properly while maintaining backward compatibility.
dunkla
force-pushed
the
claude/add-multiple-genres-01AKN5cZkyhLLwf2zh3ue8rT
branch
from
e347151 to
f6ee49a
Compare
snejus
requested changes
Nov 16, 2025
Member
snejus
left a comment
snejus
left a comment
There was a problem hiding this comment.
Hi, thanks for your contribution!
The main bottleneck that we have with a multi-value genres field is that mediafile does not currently support it. We need to add it, and make it behave the same way as, say, artists field does. I can take care of this.
Otherwise, I reckon we should simplify this change by migrating to using genres field for good.
Comment on lines
+175
to
+215
| def sync_genre_fields() -> None: | ||
| """Synchronize genre and genres fields with proper join/split logic. | ||
| The genre field stores a joined string of all genres (for backward | ||
| compatibility with users who store multiple genres as delimited strings), | ||
| while genres is the native list representation. | ||
| When multi_value_genres config is disabled, only the first genre is used. | ||
| """ | ||
| genre_val = getattr(m, "genre") | ||
| genres_val = getattr(m, "genres") | ||
|
|
||
| # Handle None values - treat as empty | ||
| if genres_val is None: | ||
| genres_val = [] | ||
| if genre_val is None: | ||
| genre_val = "" | ||
|
|
||
| if config["multi_value_genres"]: | ||
| # New behavior: sync all genres using configurable separator | ||
| separator = config["genre_separator"].get(str) | ||
| if genres_val: | ||
| # If genres list exists, join it into genre string | ||
| setattr(m, "genre", separator.join(genres_val)) | ||
| elif genre_val: | ||
| # If only genre string exists, split it into genres list | ||
| # and clean up the genre string | ||
| cleaned_genres = [ | ||
| g.strip() for g in genre_val.split(separator) if g.strip() | ||
| ] | ||
| setattr(m, "genres", cleaned_genres) | ||
| setattr(m, "genre", separator.join(cleaned_genres)) | ||
| else: | ||
| # Old behavior: only sync first value (like albumtype) | ||
| if genre_val: | ||
| setattr(m, "genres", unique_list([genre_val, *genres_val])) | ||
| elif genres_val: | ||
| setattr(m, "genre", genres_val[0]) | ||
|
|
||
| ensure_first_value("albumtype", "albumtypes") | ||
| sync_genre_fields() |
Member
There was a problem hiding this comment.
This should be simplified to:
Suggested change
| def sync_genre_fields() -> None: | |
| """Synchronize genre and genres fields with proper join/split logic. | |
| The genre field stores a joined string of all genres (for backward | |
| compatibility with users who store multiple genres as delimited strings), | |
| while genres is the native list representation. | |
| When multi_value_genres config is disabled, only the first genre is used. | |
| """ | |
| genre_val = getattr(m, "genre") | |
| genres_val = getattr(m, "genres") | |
| # Handle None values - treat as empty | |
| if genres_val is None: | |
| genres_val = [] | |
| if genre_val is None: | |
| genre_val = "" | |
| if config["multi_value_genres"]: | |
| # New behavior: sync all genres using configurable separator | |
| separator = config["genre_separator"].get(str) | |
| if genres_val: | |
| # If genres list exists, join it into genre string | |
| setattr(m, "genre", separator.join(genres_val)) | |
| elif genre_val: | |
| # If only genre string exists, split it into genres list | |
| # and clean up the genre string | |
| cleaned_genres = [ | |
| g.strip() for g in genre_val.split(separator) if g.strip() | |
| ] | |
| setattr(m, "genres", cleaned_genres) | |
| setattr(m, "genre", separator.join(cleaned_genres)) | |
| else: | |
| # Old behavior: only sync first value (like albumtype) | |
| if genre_val: | |
| setattr(m, "genres", unique_list([genre_val, *genres_val])) | |
| elif genres_val: | |
| setattr(m, "genre", genres_val[0]) | |
| ensure_first_value("albumtype", "albumtypes") | |
| sync_genre_fields() | |
| ensure_first_value("genre", "genres") | |
| ensure_first_value("albumtype", "albumtypes") |
I think we should migrate to multi-value genres for good and not provide users with an option to not to. Ultimately, this is just about how we store them in our database, so it's best to have a single way of doing it. Thus, there's no need for multi_value_genres or genre_separator options.
| 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"]] |
Member
There was a problem hiding this comment.
Given my comment above, I expect beatport plugin to simply make sure that it always provides genres list.
| for genreitem in source: | ||
| genres[genreitem["name"]] += int(genreitem["count"]) | ||
| info.genre = "; ".join( | ||
| genre_list = [ |
Member
There was a problem hiding this comment.
This should be just
info.genres = [
genre
for genre, _count in sorted(genres.items(), key=lambda g: -g[1])
]| else: | ||
| formatted = tags | ||
|
|
||
| return self.config["separator"].as_str().join(formatted) |
Member
There was a problem hiding this comment.
The adjustments in lastgenre I'd expect:
- Remove/deprecate
separatorconfiguration - Write
genresfield with a list of genres instead of joining them intogenre
Member
|
Hi @dunkla many many thanks for this contribution! It lately crosses my mind daily that I should probably finally start working on multi-genre support before continuing with my lastgenre masterplan ;-), so thanks again for your motivation to drive this forward. When I skimmed through it yesterday my first thoughts were that no user and no plugin should have to worry about population both Also @snejus do you think this PR should wait for #6165 to be finished or at least should be based on it (to let @dunkla continue workin on it). |
Member
|
Also thanks for offering to implement the |
Author
|
As soon as decisions have been made i'm more than happy to rebase and restructre the approach as wished. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 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.
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:
Backward Compatibility:
Migration:
Code Review Feedback Addressed: