Skip to content

Use None as default values for multi-valued fields in hooks.Info#6405

Merged
snejus merged 4 commits intomasterfrom
handle-empty-multi-valued-tags
Mar 3, 2026
Merged

Use None as default values for multi-valued fields in hooks.Info#6405
snejus merged 4 commits intomasterfrom
handle-empty-multi-valued-tags

Conversation

@snejus
Copy link
Member

@snejus snejus commented Mar 2, 2026

Fix empty list treated as non-empty field value in _apply_metadata

Fixes #6403

Thanks to @aereaux for reporting and helpful debugging!

Problem

In beets/autotag/__init__.py, _apply_metadata skipped overwriting existing field values with None to avoid clearing data unintentionally. However, an empty list ([]) was not treated the same way — it would still overwrite existing field values, effectively clearing multi-valued fields (e.g. genres) when the tag source returned nothing.

Fix

Keep multi-value field values as None by default in beets.autotag.hooks.Info subclasses:

def __init__(
    ...
    genres: list[str] | None = None,
    ...
):
    ...
    # Before
    self.genres = genres or []
    # After
    self.genres = genres

Test changes

  • Added genres=["Rock", "Pop"] to the test album fixture to expose the bug: album genres were not being propagated to tracks due to the empty-list issue, since empty track-level genres overwrote them.
  • Removed the @pytest.mark.xfail marker once the fix made the test pass.
  • Consolidated ~20 granular ApplyTest methods and the separate ApplyCompilationTest class into a single data-driven test_autotag_items test, reducing noise and improving coverage clarity.
  • Moved autotag tests into test/autotag/ to better reflect module structure.

@snejus snejus requested a review from a team as a code owner March 2, 2026 07:53
@github-actions
Copy link

github-actions bot commented Mar 2, 2026

Thank you for the PR! The changelog has not been updated, so here is a friendly reminder to check if you need to add an entry.

@snejus
Copy link
Member Author

snejus commented Mar 2, 2026

Note I cherry-picked the test refactor commit from #6165.

Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

PR make autotag metadata apply stop clearing multi-value fields when tag source give empty list (like genres). This fit core autotag flow so mbsync/tag update not delete existing list data by accident.

Changes:

  • Treat [] same as None in _apply_metadata unless field allowed in overwrite_null
  • Move autotag tests into test/autotag/ and collapse many small apply tests into one data-driven test
  • Update fixture to include genres=["Rock","Pop"] so empty-list bug show up in tests

Reviewed changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.

File Description
beets/autotag/init.py Skip applying empty lists so existing multi-value fields not wiped
test/test_autotag.py Remove old autotag test module (relocated)
test/autotag/test_autotag.py New autotag test module; adds genres regression coverage and consolidates apply tests

@codecov
Copy link

codecov bot commented Mar 2, 2026

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 69.33%. Comparing base (86fac4b) to head (a8b8aa9).
⚠️ Report is 5 commits behind head on master.
✅ All tests successful. No failed tests found.

Additional details and impacted files
@@           Coverage Diff           @@
##           master    #6405   +/-   ##
=======================================
  Coverage   69.33%   69.33%           
=======================================
  Files         141      141           
  Lines       18793    18793           
  Branches     3061     3061           
=======================================
  Hits        13031    13031           
  Misses       5117     5117           
  Partials      645      645           
Files with missing lines Coverage Δ
beets/autotag/hooks.py 99.21% <100.00%> (ø)
🚀 New features to boost your workflow:
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@snejus snejus force-pushed the handle-empty-multi-valued-tags branch 2 times, most recently from 8327cb3 to c892548 Compare March 2, 2026 08:39
@aereaux
Copy link
Contributor

aereaux commented Mar 2, 2026

This looks like it fixes #6403 for me, thanks!

@snejus snejus force-pushed the handle-empty-multi-valued-tags branch 2 times, most recently from c3c0bd9 to b739a1d Compare March 2, 2026 17:03
@snejus snejus changed the title Assume that empty list is an empty field Use None as default values for multi-valued fields in hooks.Info Mar 2, 2026
snejus added 4 commits March 3, 2026 07:46
Consolidate multiple granular test methods in ApplyTest into a single
comprehensive test that validates all applied metadata at once. This
improves test maintainability and clarity by:

- Replacing ~20 individual test methods with one data-driven test
- Using expected data dictionaries to validate all fields together
- Removing ApplyCompilationTest class (covered by va=True in main test)
- Keeping focused tests for edge cases (artist_credit, date handling)
- Switching from BeetsTestCase to standard TestCase for speed
- Adding operator import for efficient data extraction

The new approach makes it easier to validate all applied metadata at once.
@snejus snejus force-pushed the handle-empty-multi-valued-tags branch from b739a1d to a8b8aa9 Compare March 3, 2026 07:48
@snejus snejus merged commit 50684b7 into master Mar 3, 2026
20 checks passed
@snejus snejus deleted the handle-empty-multi-valued-tags branch March 3, 2026 11:58
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.

Musicbrainz genres deleted using mbsync after https://github.com/beetbox/beets/pull/6401

3 participants