Skip to content

Conversation

@snejus
Copy link
Member

@snejus snejus commented Oct 25, 2025

Fixes: #6121

This PR introduces a centralized deprecation system and adjusts musicbrainz plugin loading to properly handle the deprecated musicbrainz.enabled configuration option.

MusicBrainz

  • Added deprecation warnings for the musicbrainz.enabled configuration option:
    • When set to true, warns users to explicitly add musicbrainz to their plugins configuration and adds it if not already present
    • When set to false, warns users and adds the plugin to disabled_plugins (list
      received by the --disable-plugins flag)

Deprecations

  • Created new beets/util/deprecation.py module with standardized deprecation helpers:

    • deprecate_for_user() - logs warnings visible to end users
    • deprecate_for_maintainers() - emits DeprecationWarning for developers
    • deprecate_imports() - handles deprecated module imports with automatic version calculation
    • _format_message() - generates consistent deprecation messages that auto-calculate next major version
  • Migrated all deprecation handling to use the new centralized functions:

    • Replaced inline warnings.warn() calls throughout codebase
    • Updated deprecate_imports() signature to remove explicit version parameter
    • Converted user-facing deprecation warnings in plugins to use logger-based deprecate_for_user()

Copilot AI review requested due to automatic review settings October 25, 2025 13:33
@snejus snejus requested a review from a team as a code owner October 25, 2025 13:33
@github-actions
Copy link

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.

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

This PR centralizes deprecation handling in beets by introducing a new beets/util/deprecation.py module with standardized helper functions, and updates the MusicBrainz plugin loading behavior to always load unless explicitly disabled.

Key Changes:

  • Created centralized deprecation system with deprecate_for_user(), deprecate_for_maintainers(), and deprecate_imports() functions
  • Modified MusicBrainz plugin to auto-load when not in plugins list and not disabled, with deprecation warnings
  • Migrated all deprecation warnings throughout codebase to use new centralized functions

Reviewed Changes

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

Show a summary per file
File Description
beets/util/deprecation.py New module providing centralized deprecation helper functions with automatic version calculation
beets/util/init.py Removed old deprecate_imports() function migrated to new deprecation module
beetsplug/musicbrainz.py Updated to use new deprecate_for_user() function for searchlimit option warning
beets/ui/init.py Replaced inline warning with deprecate_for_maintainers() call in decargs function
beets/plugins.py Updated MusicBrainz auto-loading logic and migrated deprecation warnings to new functions
beets/mediafile.py Replaced inline warning with deprecate_for_maintainers() call
beets/library/init.py Updated import and removed version parameter from deprecate_imports() call
beets/autotag/init.py Migrated deprecation warnings to new centralized functions
beets/init.py Updated import and simplified deprecate_imports() call
beets/config_default.yaml Changed default plugins list from [musicbrainz] to [] and added disabled_plugins option

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

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!


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 Oct 25, 2025

Codecov Report

❌ Patch coverage is 75.55556% with 11 lines in your changes missing coverage. Please review.
✅ Project coverage is 67.03%. Comparing base (201677a) to head (2a18445).
✅ All tests successful. No failed tests found.

Files with missing lines Patch % Lines
beets/mediafile.py 0.00% 3 Missing ⚠️
beets/plugins.py 75.00% 2 Missing and 1 partial ⚠️
beets/util/deprecation.py 85.71% 2 Missing and 1 partial ⚠️
beets/autotag/__init__.py 50.00% 1 Missing ⚠️
beets/ui/__init__.py 50.00% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master    #6127      +/-   ##
==========================================
+ Coverage   66.98%   67.03%   +0.04%     
==========================================
  Files         118      119       +1     
  Lines       18192    18211      +19     
  Branches     3079     3082       +3     
==========================================
+ Hits        12186    12207      +21     
+ Misses       5347     5345       -2     
  Partials      659      659              
Files with missing lines Coverage Δ
beets/__init__.py 66.66% <100.00%> (ø)
beets/library/__init__.py 100.00% <100.00%> (ø)
beets/util/__init__.py 78.67% <ø> (+0.35%) ⬆️
beetsplug/musicbrainz.py 69.57% <100.00%> (+0.08%) ⬆️
beets/autotag/__init__.py 87.61% <50.00%> (-0.11%) ⬇️
beets/ui/__init__.py 79.41% <50.00%> (ø)
beets/mediafile.py 0.00% <0.00%> (ø)
beets/plugins.py 84.21% <75.00%> (+1.08%) ⬆️
beets/util/deprecation.py 85.71% <85.71%> (ø)
🚀 New features to boost your workflow:
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@henry-oberholtzer
Copy link
Member

I like the additional deprecation warnings, but adding a disabled plugin list doesn't feel right to me when a plugin should be considered disabled by merit of not being on the list of plugins to begin with.

My thoughts on a solution to the Musicbrainz problem:

Throw an error, and ask the user if they'd like to use musicbrainz.

I'm not sure how possible it is for us to modify the user's config file, but if possible we could just add it to the list of enabled plugins and then the warning shouldn't appear again, unless the user intentionally switches for just discogs, or spotify, or similar.

@snejus snejus changed the title Always enable mb Add deprecation warning for musicbrainz.enabled but use it to load the plugin, centralise deprecations handling Oct 26, 2025
@snejus
Copy link
Member Author

snejus commented Oct 26, 2025

I like the additional deprecation warnings, but adding a disabled plugin list doesn't feel right to me when a plugin should be considered disabled by merit of not being on the list of plugins to begin with.

Agree with you. I have now updated the logic and added a test to show the behaviour (otherwise there's too much to wrap one's head around!)

},
name=name,
version="3.0.0",
__name__, {"art": "beetsplug._utils", "vfs": "beetsplug._utils"}, name
Copy link
Contributor

@semohr semohr Oct 26, 2025

Choose a reason for hiding this comment

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

Could you try to keep the trailing commas here? I recon we might add more imports here in the future.

See COM812:
The presence of a trailing comma can reduce diff size when parameters or elements are added or removed from function calls, function definitions, literals, etc.

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.

Certain albums are not searched on musicbrainz with any plugin enabled?

3 participants