Skip to content

Conversation

@OSokunbi
Copy link

Description

Fixes #6068

  • refactored the Discogs importer to build canonical/ANV artist credit parts once and reuse them for both releases and per-track data, so every multi-value field in AlbumInfo/TrackInfo is now populated (artists, artist_credit, artists_ids, artists_sort, albumtypes, featured performers).
  • track parsing now shares the album credit logic, switches to per-track credits, and preserves featured performers for each track.
  • added regression coverage in test/plugins/test_discogs.py to assert album/track multi-value fields, IDs, and ANV/featured combinations.

To Do

  • Documentation.
  • Changelog.
  • Tests. (python -m pytest test/plugins/test_discogs.py)

Copilot AI review requested due to automatic review settings December 10, 2025 23:28
@OSokunbi OSokunbi requested a review from a team as a code owner December 10, 2025 23:28
@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

@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 - here's some feedback:

  • The ArtistCreditParts/ArtistCreditData flow is repeated in several places (album, track, featured logic); consider a small helper that returns the selected variant (default vs ANV) for artist, artist_credit, and their lists to reduce duplication and branching.
  • In get_album_info, albumtypes_list is derived only from formats[0]; if multiple formats are present, you may want to either merge descriptions from all formats or document that only the first is used to avoid surprising behavior.
  • The cast(list[Artist], featured_list) in get_track_info suggests the type of featured_list is not aligned with Artist; consider tightening the type of featured_list or broadening the accepted type of _build_artist_credit_data to avoid the need for a cast.
Prompt for AI Agents
Please address the comments from this code review:

## Overall Comments
- The ArtistCreditParts/ArtistCreditData flow is repeated in several places (album, track, featured logic); consider a small helper that returns the selected variant (default vs ANV) for `artist`, `artist_credit`, and their lists to reduce duplication and branching.
- In `get_album_info`, `albumtypes_list` is derived only from `formats[0]`; if multiple formats are present, you may want to either merge descriptions from all formats or document that only the first is used to avoid surprising behavior.
- The `cast(list[Artist], featured_list)` in `get_track_info` suggests the type of `featured_list` is not aligned with `Artist`; consider tightening the type of `featured_list` or broadening the accepted type of `_build_artist_credit_data` to avoid the need for a cast.

## Individual Comments

### Comment 1
<location> `beetsplug/discogs.py:501-506` </location>
<code_context>
             va_name = config["va_name"].as_str()
             album_artist = va_name
             artist_credit = va_name
+            album_artists = [va_name]
+            album_artists_credit = [va_name]
+            album_artists_sort = [va_name]
         if catalogno == "none":
             catalogno = None
</code_context>

<issue_to_address>
**issue (bug_risk):** Consider resetting `album_artists_ids` when overriding all VA-related artist fields.

In this VA path we overwrite all artist name fields with `va_name`, but keep the original `album_artists_ids`. That breaks the expected alignment between names and IDs for consumers that treat these as parallel arrays. Consider either clearing `album_artists_ids` here or setting it to a single VA-specific ID (if one exists) to keep them consistent.
</issue_to_address>

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.

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 refactors the Discogs metadata plugin to properly support multi-value album and track fields (artists, artists_credit, artists_ids, artists_sort, albumtypes). Previously, these fields were not being populated for AlbumInfo and TrackInfo objects.

Key changes:

  • Introduced ArtistCreditParts and ArtistCreditData named tuples to build canonical and ANV artist credits once and reuse them throughout the parsing process
  • Track parsing now inherits from album credit logic, can switch to per-track credits when present, and preserves featured performers for each track
  • Added comprehensive test coverage for multi-value fields, ANV handling, and featured artist combinations

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.

File Description
beetsplug/discogs.py Refactored artist credit handling with new data structures; added multi-value field support to album and track info generation; improved featured artist handling to populate multi-value fields
test/plugins/test_discogs.py Added new imports for data structures; added test_populates_multi_value_fields to verify multi-value field population; updated test_anv to assert multi-value fields; updated test_parse_featured_artists to use new ArtistCreditData parameter structure

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

Comment on lines +482 to +486
media, albumtype = self.get_media_and_albumtype(formats)
albumtypes_list: list[str] = []
if formats and (first_format := formats[0]):
if descriptions := first_format.get("descriptions"):
albumtypes_list = list(descriptions)
Copy link

Copilot AI Dec 10, 2025

Choose a reason for hiding this comment

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

The albumtypes extraction logic is duplicated. The method get_media_and_albumtype already extracts descriptions from formats, but then lines 483-486 extract the same information again separately. Consider either:

  1. Modifying get_media_and_albumtype to return a third value (the list of descriptions), or
  2. Extracting albumtypes_list directly from the albumtype string returned by get_media_and_albumtype by splitting on ", ".
Suggested change
media, albumtype = self.get_media_and_albumtype(formats)
albumtypes_list: list[str] = []
if formats and (first_format := formats[0]):
if descriptions := first_format.get("descriptions"):
albumtypes_list = list(descriptions)
media, albumtype, albumtypes_list = self.get_media_and_albumtype(formats)

Copilot uses AI. Check for mistakes.
artist_credit = va_name
album_artists = [va_name]
album_artists_credit = [va_name]
album_artists_sort = [va_name]
Copy link

Copilot AI Dec 10, 2025

Choose a reason for hiding this comment

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

When handling Various Artists releases, the code resets album_artist, artist_credit, album_artists, album_artists_credit, and album_artists_sort to the VA name, but it does not reset album_artists_ids. This leaves the IDs from the original artists, which is inconsistent. For VA releases, album_artists_ids should be reset to an empty list, since the VA name typically doesn't have an associated artist ID.

Suggested change
album_artists_sort = [va_name]
album_artists_sort = [va_name]
album_artists_ids = []

Copilot uses AI. Check for mistakes.
@codecov
Copy link

codecov bot commented Dec 10, 2025

Codecov Report

❌ Patch coverage is 87.65432% with 10 lines in your changes missing coverage. Please review.
✅ Project coverage is 67.94%. Comparing base (ffede9d) to head (32c19ce).
✅ All tests successful. No failed tests found.

Files with missing lines Patch % Lines
beetsplug/discogs.py 87.65% 4 Missing and 6 partials ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master    #6214      +/-   ##
==========================================
+ Coverage   67.90%   67.94%   +0.04%     
==========================================
  Files         137      137              
  Lines       18689    18741      +52     
  Branches     3160     3166       +6     
==========================================
+ Hits        12690    12733      +43     
- Misses       5332     5336       +4     
- Partials      667      672       +5     
Files with missing lines Coverage Δ
beetsplug/discogs.py 72.16% <87.65%> (+1.37%) ⬆️
🚀 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

Hi! Thanks for the PR, we've already got a PR open at the moment that addresses this issue & a few others: #6179

If you have any suggestions for that PR I'd be happy to look into incorporating them, but if my approach doesn't end up working we can circle back to this.

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.

discogs: add support for multi value fields

2 participants