-
Notifications
You must be signed in to change notification settings - Fork 1.9k
Refactor of metadata plugin and opt in all metadata plugins to new baseclass #5787
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
Conversation
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. |
There was a problem hiding this 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 refactors the metadata lookup architecture by introducing a new beets.metadata_plugins
module with a single base interface and migrating all existing source plugins to use it.
- Added
beets/metadata_plugins.py
definingMetadataSourcePluginNext
andSearchApiMetadataSourcePluginNext
- Updated all core and third-party plugins (Spotify, MusicBrainz, Discogs, Deezer, Beatport, Chroma/Acoustid, mbsync, missing) to inherit from the new base classes
- Adjusted tests and documentation to patch and reference
beets.metadata_plugins
instead of the oldbeets.plugins
metadata methods
Reviewed Changes
Copilot reviewed 17 out of 17 changed files in this pull request and generated 3 comments.
Show a summary per file
File | Description |
---|---|
test/test_ui.py | Adjusted plugin-loading assertion to use new API |
test/test_importer.py | Patches updated to beets.metadata_plugins lookups |
test/plugins/test_mbsync.py | References to plugins.track_for_id /album_for_id replaced |
docs/dev/plugins.rst | Documentation revised for new metadata plugin interface |
docs/dev/index.rst | Added Confuse library link |
beets/util/id_extractors.py | Fixed key lookup to use lowercase source names |
beets/plugins.py | Removed old metadata plugin base and helper methods |
beets/metadata_plugins.py | New metadata plugin interface and loader functions |
beetsplug/spotify.py | Migrated Spotify plugin to SearchApiMetadataSourcePluginNext |
beetsplug/musicbrainz.py | Updated MusicBrainz plugin to MetadataSourcePluginNext |
beetsplug/missing.py | Switched plugins.* calls to metadata_plugins.* |
beetsplug/mbsync.py | Updated track/album lookup to new API |
beetsplug/discogs.py | Refactored Discogs plugin to new metadata interface |
beetsplug/deezer.py | Migrated Deezer plugin to SearchApiMetadataSourcePluginNext |
beetsplug/chroma.py | Adapted Acoustid plugin to new base and import patterns |
beetsplug/beatport.py | Refactored Beatport plugin to new base class and models |
Comments suppressed due to low confidence (2)
test/test_ui.py:1062
- [nitpick] The variable name
plugs
is ambiguous; consider renaming tofound_plugins
orloaded_plugins
for clarity.
plugs = plugins.find_plugins()
beetsplug/chroma.py:29
TrackInfo
is not exported bybeets.metadata_plugins
at runtime; import it frombeets.autotag.hooks
instead.
from beets.metadata_plugins import MetadataSourcePluginNext, TrackInfo
This comment was marked as resolved.
This comment was marked as resolved.
c252adc
to
2f0b610
Compare
This comment was marked as resolved.
This comment was marked as resolved.
fixed minor suggestions from copilot review
Description
At the moment the
MetaDataSourcePlugin
has multiple responsibilities:_search
apiI propose splitting these responsibilities, as it would enable us to use the
MetaDataSourcePlugin
baseclass with plugins that use external packages to fetch data.This follows from discussion in #5761 and #5748 (comment).
Feedback is highly appreciated, as this is mainly architectural decision and I would prefer if the new behavior is a shared consensus.
To Do
MetaDataSourcePlugin
This PR was initially #5764 and was accidentally closed as the target branch was deleted. Wasn't able to recover the original PR.