Refactor of metadata plugin and opt in all metadata plugins to new baseclass

[semohr]

Description

At the moment the MetaDataSourcePlugin has multiple responsibilities:

• fetch data via _search api
• defines contract for interaction within the beets autotag lookup

I 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 https://github.com/beetbox/beets/pull/5748#discussion_r2075070638.

Feedback is highly appreciated, as this is mainly architectural decision and I would prefer if the new behavior is a shared consensus.

To Do

• [x] Opt in plugins into the new MetaDataSourcePlugin
  • [x] Spotify
  • [x] Musicbrainz
  • [x] Deezer
  • [x] Beatport
  • [x] Chroma
  • [x] Disccogs
• [x] Remove old MetaDataSourcePlugin and related functions
• [x] Documentation on the ontology of plugins
• [x] Changelog

This PR was initially #5764 and was accidentally closed as the target branch was deleted. Wasn't able to recover the original PR.

[github-actions[bot]]

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]

You may want to consider updating your (dependent) branches with git rebase + force pushes rather than git merge. GitHub seems to support it a bit better, for example, see these events under https://github.com/beetbox/beets/pull/5761: it automatically updated the base branch from make-musicbrainz-a-plugin to master at the point make-musicbrainz-a-plugin got merged to master.

[semohr]

You may want to consider updating your (dependent) branches with git rebase + force pushes rather than git merge. GitHub seems to support it a bit better, for example, see these events under #5761: it automatically updated the base branch from make-musicbrainz-a-plugin to master at the point make-musicbrainz-a-plugin got merged to master.

Was already on it, before I have seen this :+1: Should be way cleaner now.

[semohr]

Good stuff! I added a couple of comments in the interfaces, however I'm unable to see how this affects the plugins because they have also been refactored.

Could you move optional changes to a separate PR to minimize any side effects this change may cause? I tried testing it and I found spotify stuck in some authentication exception loop, for example:

...done.
Success. Distance: 0.58
Sending event: albuminfo_received
Candidate: DRS - Mid Mic Crisis (6817507)
Computing track assignment...
...done.
Success. Distance: 0.58
spotify: Searching Spotify for 'album:Skeptical Answers artist:Vulkanski'
spotify: Spotify access token has expired. Reauthenticating.
spotify: Spotify access token has expired. Reauthenticating.
spotify: Spotify access token has expired. Reauthenticating.
spotify: Spotify access token has expired. Reauthenticating.
spotify: Spotify access token has expired. Reauthenticating.
spotify: Spotify access token has expired. Reauthenticating.
spotify: Spotify access token has expired. Reauthenticating.
spotify: Spotify access token has expired. Reauthenticating.
spotify: Spotify access token has expired. Reauthenticating.
spotify: Spotify access token has expired. Reauthenticating.
spotify: Spotify access token has expired. Reauthenticating.

That is a good point. How about we keep it for now (at least) until we are happy with the metadata_plugin.py file and after that I slowly start to create PRs to prepare the specific metadataplugins for adaption. Once all plugins are migrated we should than be able to merge this PR.

[snejus]

slowly start to create PRs to prepare the specific metadata plugins for adaption

If you undo MetadataSourcePlugin.get_artist -> artists_to_artist_str replacement and _get_id removal, you should be left with very little that needs adapting.

[snejus]

Can we keep this PR focused on what the title suggests?

Refactor of metadata plugin and opt in all metadata plugins to new baseclass

Could you move commits with plugins and documentation improvements to separate branches, so that we can review them separately? I'm keen to merge the changes that have been reviewed and would like to have a final look without any noise.

[semohr]

I will have another look here and rebase it after we merge the #5814 PR.

[semohr]

Moved the docs changes into #5861. Feel free to have another look. Just rebased onto master and should good to go from my side.

[snejus]

Thanks for moving the docs changes out! The PR still contains plugin refactors, namely beatport, deezer and spotify. I see the changes you've made in discogs and I expect an equivalent amount of changes to be made in the rest of the plugins, not more - to keep them focused on the new functionality.

[semohr]

While beatport has seen more significant updates, changes to the other plugins are minimal and primarily related to the MetadataSourcePlugin interface.

For the spotify plugin, I introduced stricter types for query responses. These changes do not affect the core logic and are closely related to the MetadataSourcePlugin improvements, so I believe they belong in this PR.

We can drop the beatport changes if you'd prefer, though I probably won’t move them to a separate PR as I’m not using Beatport myself, just noticed some type issues that needed fixing.

[semohr]

This may be my last comment 😅 Check the build for typing error, I think there's one popping up:

Argument 1 to "append" of "list" has incompatible type "BeetsPlugin"; expected "MetadataSourcePlugin"

Seemly like this was an issue introduced with the legacy support. Sadly the fix needs a type ignore but we should be able to remove it with v3.0.0.

[snejus]

Just rebase it and it will get merged.