-
Notifications
You must be signed in to change notification settings - Fork 2k
Add support multi-value album/track fields in discogs plugin #6214
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.
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_listis derived only fromformats[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)inget_track_infosuggests the type offeatured_listis not aligned withArtist; consider tightening the type offeatured_listor broadening the accepted type of_build_artist_credit_datato 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>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
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 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
ArtistCreditPartsandArtistCreditDatanamed 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.
| 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) |
Copilot
AI
Dec 10, 2025
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.
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:
- Modifying
get_media_and_albumtypeto return a third value (the list of descriptions), or - Extracting albumtypes_list directly from the albumtype string returned by
get_media_and_albumtypeby splitting on ", ".
| 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) |
| artist_credit = va_name | ||
| album_artists = [va_name] | ||
| album_artists_credit = [va_name] | ||
| album_artists_sort = [va_name] |
Copilot
AI
Dec 10, 2025
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.
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.
| album_artists_sort = [va_name] | |
| album_artists_sort = [va_name] | |
| album_artists_ids = [] |
Codecov Report❌ Patch coverage is
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
🚀 New features to boost your workflow:
|
|
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. |
Description
Fixes #6068
AlbumInfo/TrackInfois now populated (artists, artist_credit, artists_ids, artists_sort, albumtypes, featured performers).test/plugins/test_discogs.pyto assert album/track multi-value fields, IDs, and ANV/featured combinations.To Do