Skip to content

Conversation

@dunkla
Copy link

@dunkla dunkla commented Nov 16, 2025

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 dunkla requested a review from a team as a code owner November 16, 2025 18:17
Copy link
Contributor

@sourcery-ai sourcery-ai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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>

Sourcery is free for open source - if you like our reviews please consider sharing them ✨
Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.

@codecov
Copy link

codecov bot commented Nov 16, 2025

Codecov Report

❌ Patch coverage is 54.87805% with 37 lines in your changes missing coverage. Please review.
✅ Project coverage is 67.86%. Comparing base (2bd77b9) to head (c3f8eac).
✅ All tests successful. No failed tests found.

Files with missing lines Patch % Lines
beets/ui/commands/migrate.py 22.50% 31 Missing ⚠️
beetsplug/lastgenre/__init__.py 78.94% 3 Missing and 1 partial ⚠️
beets/autotag/__init__.py 92.30% 0 Missing and 1 partial ⚠️
beetsplug/beatport.py 83.33% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master    #6169      +/-   ##
==========================================
- Coverage   67.93%   67.86%   -0.07%     
==========================================
  Files         137      138       +1     
  Lines       18677    18733      +56     
  Branches     3155     3167      +12     
==========================================
+ Hits        12688    12713      +25     
- Misses       5324     5355      +31     
  Partials      665      665              
Files with missing lines Coverage Δ
beets/autotag/hooks.py 100.00% <100.00%> (ø)
beets/library/models.py 87.17% <ø> (ø)
beets/ui/commands/__init__.py 100.00% <100.00%> (ø)
beetsplug/musicbrainz.py 79.22% <100.00%> (+0.05%) ⬆️
beets/autotag/__init__.py 88.09% <92.30%> (+0.48%) ⬆️
beetsplug/beatport.py 44.08% <83.33%> (+0.27%) ⬆️
beetsplug/lastgenre/__init__.py 72.18% <78.94%> (+0.43%) ⬆️
beets/ui/commands/migrate.py 22.50% <22.50%> (ø)
🚀 New features to boost your workflow:
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@dunkla dunkla force-pushed the claude/add-multiple-genres-01AKN5cZkyhLLwf2zh3ue8rT branch from e347151 to f6ee49a Compare November 16, 2025 19:46
Copy link
Member

@snejus snejus left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@JOJ0
Copy link
Member

JOJ0 commented Nov 17, 2025

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 genre and genres, so I'd like to second @snejus here: Writing a genres list should be the only thing that any Beets plugin and user should have to do. Any fallback/opt-in/opt-out scenarios are not necessary and complicate things.

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).

@JOJ0
Copy link
Member

JOJ0 commented Nov 17, 2025

Also thanks for offering to implement the mediafile end @snejus! I think we should finalize the current cleanup first though. Then implement genres first thing! beetbox/mediafile#86

@dunkla
Copy link
Author

dunkla commented Nov 21, 2025

As soon as decisions have been made i'm more than happy to rebase and restructre the approach as wished.

@snejus
Copy link
Member

snejus commented Dec 5, 2025

Good news, guys! Apparently, mediafile already supports multivalued genres field! @dunkla you can go ahead and implement the requested changes whenever you have time.

Additionally, I think, we should think of the best way to migrate joined genres from the existing genre field to the new field at the point of this field is created. Unlike in #5540, we have an opportunity to do this properly this time, where we don't need to depend on mbsync / reimports!

dunkla pushed a commit to dunkla/beets that referenced this pull request Dec 6, 2025
Simplify multi-genre implementation based on maintainer feedback (PR beetbox#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 beetbox#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
@dunkla dunkla force-pushed the claude/add-multiple-genres-01AKN5cZkyhLLwf2zh3ue8rT branch from f6ee49a to 4e2f78d Compare December 6, 2025 23:27
@dunkla
Copy link
Author

dunkla commented Dec 6, 2025

Summary of Changes

Addressed PR Comments

1. Simplify the architecture (snejus's main comment)

  • Removed multi_value_genres config option from beets/config_default.yaml
  • Removed genre_separator config option from beets/config_default.yaml
  • Replaced complex sync_genre_fields() with simple ensure_first_value("genre", "genres") call in beets/autotag/init.py:204
  • Simplified all plugins to always write genres as lists (no conditional logic based on config)

2. Update Beatport plugin (snejus's comment)

  • Simplified beetsplug/beatport.py:236-239 to always populate genres list
  • Removed all conditional multi_value_genres config checks from BeatportRelease and BeatportTrack

3. Update MusicBrainz plugin (snejus's comment)

  • Simplified beetsplug/musicbrainz.py:739-743 to write directly to info.genres as list
  • Removed config-based conditional logic

4. Update LastGenre plugin (snejus's comment)

  • Major refactor of beetsplug/lastgenre/init.py:
    • Changed _get_genre() to return list instead of string
    • Renamed _format_and_stringify() to _format_genres() returning list
    • Removed all separator-related configuration logic
    • Simplified _get_existing_genres() to only read from genres field
    • Updated _fetch_and_log_genre() to write directly to obj.genres

Addressed Issues

Migration concerns (referenced in PR discussion, relates to #5540)

  • Added automatic lazy migration in beets/autotag/init.py:167-186
    • Detects comma (", "), semicolon ("; "), and slash (" / ") separated genre strings
    • Automatically splits into genres list on first item access
    • No reimport or mbsync needed for existing libraries
  • Added explicit beet migrate genres command for batch processing in beets/ui/commands/migrate.py
    • Supports --pretend flag to preview changes
    • Allows users to migrate entire library at once if preferred
  • Migration strategy avoids endless rewrite loops from Stop perpetually writing mb_artistid, mb_albumartistid and albumtypes fields #5540:
    • Proper field synchronization using ensure_first_value()
    • Clears old genre field after splitting to prevent duplication
    • No bidirectional sync complexity

Other Changes

Tests

  • Rewrote all genre sync tests in test/test_autotag.py:480-590
  • Added 5 new migration test cases covering different separator types
  • Updated LastGenre tests in test/plugins/test_lastgenre.py to expect lists instead of strings
  • Updated Beatport tests in test/plugins/test_beatport.py to check .genres attribute
  • Fixed library tests in test/test_library.py to work with new field sync
  • All 44 genre-related tests passing

Documentation

  • Updated docs/changelog.rst:15-29 with simplified feature description
  • Added comprehensive migration documentation mentioning both automatic and manual options
  • Removed documentation for multi_value_genres and genre_separator config options from docs/reference/config.rst

Code Quality

  • All linter checks passing (ruff)
  • All type checks passing

Implementation Philosophy

The simplified implementation aligns with the maintainer's vision:

  • Always use multi-value genres internally
  • Automatic backward-compatible sync to genre field via ensure_first_value()
  • No configuration complexity
  • Clean migration path for existing users
  • Consistent behavior across all metadata sources

all again with immense help of claude code

@dunkla dunkla force-pushed the claude/add-multiple-genres-01AKN5cZkyhLLwf2zh3ue8rT branch from 4e2f78d to cfa9f1a Compare December 6, 2025 23:36
dunkla pushed a commit to dunkla/beets that referenced this pull request Dec 6, 2025
Simplify multi-genre implementation based on maintainer feedback (PR beetbox#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 beetbox#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
@dunkla dunkla force-pushed the claude/add-multiple-genres-01AKN5cZkyhLLwf2zh3ue8rT branch from cfa9f1a to 32e47d2 Compare December 6, 2025 23:37
dunkla pushed a commit to dunkla/beets that referenced this pull request Dec 6, 2025
Simplify multi-genre implementation based on maintainer feedback (PR beetbox#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 beetbox#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
@dunkla dunkla force-pushed the claude/add-multiple-genres-01AKN5cZkyhLLwf2zh3ue8rT branch from 32e47d2 to 22cdda7 Compare December 6, 2025 23:39
dunkla pushed a commit to dunkla/beets that referenced this pull request Dec 6, 2025
Simplify multi-genre implementation based on maintainer feedback (PR beetbox#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 beetbox#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
@dunkla dunkla force-pushed the claude/add-multiple-genres-01AKN5cZkyhLLwf2zh3ue8rT branch from 22cdda7 to fa98904 Compare December 6, 2025 23:47
Simplify multi-genre implementation based on maintainer feedback (PR beetbox#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 beetbox#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
@dunkla dunkla force-pushed the claude/add-multiple-genres-01AKN5cZkyhLLwf2zh3ue8rT branch from fa98904 to c3f8eac Compare December 6, 2025 23:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants