Skip to content
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

mbsync / missing plugins: consider all metadata backends #5636

Open
wants to merge 5 commits into
base: master
Choose a base branch
from

Conversation

snejus
Copy link
Member

@snejus snejus commented Feb 16, 2025

This PR adds support for alternate metadata backends in the mbsync and missing plugins, allowing them to work with any metadata backend the items are tagged with instead of being limited to just MusicBrainz.

Key changes:

  • Removed redundant albums_for_id and items_for_id methods.
  • Updated album_for_id and track_for_id implementations to return a single album/track as their names suggest.
  • Updated both plugins to use the above functions instead of MusicBrainz-specific ones.
  • Updated missing plugin docs to make it clear that missing albums for artists can only be obtained for the musicbrainz metadata backend
    • This is possible with other backends but implementation was outside of this PR's scope.
    • Slightly simplified the existing implementation

The changes maintain backwards compatibility while enabling users to leverage metadata from sources like Deezer, Discogs or Bandcamp when syncing their libraries.

Note: this change came about because I'm working on making musicbrainz a separate plugin: this will be submitted in the next PR.

@snejus snejus requested review from wisp3rwind and JOJ0 February 16, 2025 12:41
@snejus snejus changed the title \mbsync\ / \missing\ plugins: consider all metadata backends mbsync / missing plugins: consider all metadata backends Feb 16, 2025
@snejus snejus force-pushed the missing-mbsync-consider-all-data-sources branch from 85a442f to 46e822e Compare February 17, 2025 23:38
@wisp3rwind
Copy link
Member

While I won't have time to review this in depth anytime soon, here's one thing that's unclear to me at a superficial glance:

  • Removed redundant albums_for_id and items_for_id methods.
  • Updated album_for_id and track_for_id implementations to return a single album/track as their names suggest.

Looking just at the changed code, it appears that albums_for_id could previously indeed return several candidates given a single search_id, whereas now only the first one is used (i.e. musicbrainz if available, else the first plugin match).

Maybe this makes sense (possibly, a given search_id is only ever expected to return a single result for a single service?), but I feel like it would warrant a code comment explaining why we potentially drop additional results.

@snejus snejus force-pushed the missing-mbsync-consider-all-data-sources branch from 46e822e to d960f36 Compare March 2, 2025 22:50
@snejus
Copy link
Member Author

snejus commented Mar 2, 2025

Maybe this makes sense (possibly, a given search_id is only ever expected to return a single result for a single service?), but I feel like it would warrant a code comment explaining why we potentially drop additional results.

That's exactly my thinking - I have worked with all available metadata sources, and I'm yet to discover a case where a single ID yields more than one album / singleton. I will add a comment under these methods.

@snejus snejus force-pushed the missing-mbsync-consider-all-data-sources branch from d960f36 to 5407d82 Compare March 2, 2025 23:03
@snejus snejus force-pushed the missing-mbsync-consider-all-data-sources branch from 5407d82 to e2f7af5 Compare March 12, 2025 07:26
@snejus snejus force-pushed the missing-mbsync-consider-all-data-sources branch from e2f7af5 to 371a369 Compare March 12, 2025 07:28
@snejus snejus requested a review from Copilot March 20, 2025 21:28
Copy link

@Copilot 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 extends the mbsync and missing plugins to support alternate metadata backends beyond MusicBrainz while simplifying some redundant functionality. Key changes include:

  • Removing redundant functions and updating album/track lookup functions to return a single result.
  • Refactoring test code in mbsync and updating type annotations in missing to group album IDs.
  • Adjusting logging levels and minor string formatting in plugins such as deezer and discogs to improve clarity.

Reviewed Changes

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

Show a summary per file
File Description
test/plugins/test_mbsync.py Updated patch decorators and log message assertions.
beetsplug/missing.py Added type annotations and modified album grouping logic.
beets/autotag/hooks.py Refactored album/track lookup functions and removed redundant generators.
beets/plugins.py Updated album/track lookup functions to return single objects.
beetsplug/deezer.py Changed error log level from error to debug.
beetsplug/discogs.py Removed unused client version checks and cleaned up log messages.
beetsplug/mbsync.py Updated to use the new backend lookup functions and fixed minor log formatting.
test/plugins/test_discogs.py Patched out the setup call for discogs to avoid real API calls.
Files not reviewed (3)
  • docs/changelog.rst: Language not supported
  • docs/plugins/mbsync.rst: Language not supported
  • docs/plugins/missing.rst: Language not supported
Comments suppressed due to low confidence (1)

beetsplug/mbsync.py:86

  • The log message uses the same placeholder twice. It likely should display different values (for example, one for the recording ID and one for additional context such as the track title).
            self._log.info("Recording ID not found: {0} for track {0}", item.mb_trackid)

@JOJ0
Copy link
Member

JOJ0 commented Mar 25, 2025

Played around a little with mbsync Discogs and bandcamp. The latter keeps showing this. Not sure what this field is:

$ beet mbsync alves
Alves - Episode 05 - 01 - Dark Era Of Amnesia (Resurrection) (2016)  [] deep house, experimental, industrial, techno
  track_alt:
Alves - Episode 05 - 02 - Black Mamba (2016)  [] deep house, experimental, industrial, techno
  track_alt:
Alves - Episode 05 - 03 - Viper (2016)  [] deep house, experimental, industrial, techno
  track_alt:
Alves - Episode 05 - 04 - Problem Magnet (2016)  [] deep house, experimental, industrial, techno
  track_alt:
Alves - Episode 05 - 05 - ADHD (2016)  [] deep house, experimental, industrial, techno
  track_alt:
Alves - Episode 05 - 06 - The Chase (2016)  [] deep house, experimental, industrial, techno
  track_alt:

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.

3 participants