Discogs featured artist fix #6040
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Reviewer's GuideExtends the Discogs plugin to propagate album artist context through track parsing, implement parsing and formatting of featured artists from extraartists, add corresponding tests, and update the changelog. File-Level Changes
Assessment against linked issues
Possibly linked issues
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey there - I've reviewed your changes - here's some feedback:
- Role matching for featured artists uses a case-sensitive substring search on "Featuring"; consider normalizing the role string (e.g. lowercasing) and splitting on commas so you catch variants like "featuring" or "Feat." consistently.
- Propagating album_artist and album_artist_id through every get_tracks/get_track_info call makes the signatures bulky; you could store those defaults on the plugin instance (e.g. self.album_artist) to simplify the method interfaces.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- Role matching for featured artists uses a case-sensitive substring search on "Featuring"; consider normalizing the role string (e.g. lowercasing) and splitting on commas so you catch variants like "featuring" or "Feat." consistently.
- Propagating album_artist and album_artist_id through every get_tracks/get_track_info call makes the signatures bulky; you could store those defaults on the plugin instance (e.g. self.album_artist) to simplify the method interfaces.
## Individual Comments
### Comment 1
<location> `beetsplug/discogs.py:661-665` </location>
<code_context>
length = self.get_track_length(track["duration"])
+ # Add featured artists
+ extraartists = track.get("extraartists", [])
+ featured = [
+ artist["name"]
+ for artist in extraartists
+ if artist["role"].find("Featuring") != -1
+ ]
+ if featured:
</code_context>
<issue_to_address>
**suggestion:** The featured artist detection relies on substring matching, which may miss variations or introduce false positives.
The current approach may miss cases like 'Feat.' or 'feat.' and could match unrelated roles. Normalize the role string and use a case-insensitive check against a defined set of accepted role names for more accurate detection.
```suggestion
# Detect featured artists using normalized, case-insensitive role matching
FEATURED_ROLES = {"featuring", "feat.", "feat", "ft.", "ft"}
featured = [
artist["name"]
for artist in extraartists
if any(
role in artist.get("role", "").lower().replace(" ", "")
for role in FEATURED_ROLES
)
]
```
</issue_to_address>
### Comment 2
<location> `beetsplug/discogs.py:653-656` </location>
<code_context>
track.get("artists", []), join_key="join"
)
+ # If no artist and artist is returned, set to match album artist
+ if not artist:
+ artist = album_artist
+ artist_id = album_artist_id
artist = self.strip_disambiguation(artist)
length = self.get_track_length(track["duration"])
</code_context>
<issue_to_address>
**suggestion (bug_risk):** The fallback to album artist and artist_id may overwrite valid but empty values.
Check 'artist' and 'artist_id' separately to prevent overwriting valid values when only one is missing.
```suggestion
# Fallback to album artist and artist_id separately if missing
if not artist:
artist = album_artist
if not artist_id:
artist_id = album_artist_id
```
</issue_to_address>
### Comment 3
<location> `test/plugins/test_discogs.py:455-464` </location>
<code_context>
+ assert t.artist == expected_artist
+
+
@pytest.mark.parametrize(
"formats, expected_media, expected_albumtype",
[
</code_context>
<issue_to_address>
**suggestion (testing):** Good coverage of featured artist parsing, but missing edge cases for delimiters and empty/invalid extraartists.
Add tests for cases with no extraartists, empty or None roles, and variations in role casing or delimiters to improve robustness against Discogs data variations.
</issue_to_address>
### Comment 4
<location> `beetsplug/discogs.py:445` </location>
<code_context>
def get_tracks(self, tracklist, album_artist, album_artist_id):
"""Returns a list of TrackInfo objects for a discogs tracklist."""
try:
clean_tracklist = self.coalesce_tracks(tracklist)
except Exception as exc:
# FIXME: this is an extra precaution for making sure there are no
# side effects after #2222. It should be removed after further
# testing.
self._log.debug("{}", traceback.format_exc())
self._log.error("uncaught exception in coalesce_tracks: {}", exc)
clean_tracklist = tracklist
tracks = []
index_tracks = {}
index = 0
# Distinct works and intra-work divisions, as defined by index tracks.
divisions, next_divisions = [], []
for track in clean_tracklist:
# Only real tracks have `position`. Otherwise, it's an index track.
if track["position"]:
index += 1
if next_divisions:
# End of a block of index tracks: update the current
# divisions.
divisions += next_divisions
del next_divisions[:]
track_info = self.get_track_info(
track, index, divisions, album_artist, album_artist_id
)
track_info.track_alt = track["position"]
tracks.append(track_info)
else:
next_divisions.append(track["title"])
# We expect new levels of division at the beginning of the
# tracklist (and possibly elsewhere).
try:
divisions.pop()
except IndexError:
pass
index_tracks[index + 1] = track["title"]
# Fix up medium and medium_index for each track. Discogs position is
# unreliable, but tracks are in order.
medium = None
medium_count, index_count, side_count = 0, 0, 0
sides_per_medium = 1
# If a medium has two sides (ie. vinyl or cassette), each pair of
# consecutive sides should belong to the same medium.
if all([track.medium is not None for track in tracks]):
m = sorted({track.medium.lower() for track in tracks})
# If all track.medium are single consecutive letters, assume it is
# a 2-sided medium.
if "".join(m) in ascii_lowercase:
sides_per_medium = 2
for track in tracks:
# Handle special case where a different medium does not indicate a
# new disc, when there is no medium_index and the ordinal of medium
# is not sequential. For example, I, II, III, IV, V. Assume these
# are the track index, not the medium.
# side_count is the number of mediums or medium sides (in the case
# of two-sided mediums) that were seen before.
medium_is_index = (
track.medium
and not track.medium_index
and (
len(track.medium) != 1
or
# Not within standard incremental medium values (A, B, C, ...).
ord(track.medium) - 64 != side_count + 1
)
)
if not medium_is_index and medium != track.medium:
side_count += 1
if sides_per_medium == 2:
if side_count % sides_per_medium:
# Two-sided medium changed. Reset index_count.
index_count = 0
medium_count += 1
else:
# Medium changed. Reset index_count.
medium_count += 1
index_count = 0
medium = track.medium
index_count += 1
medium_count = 1 if medium_count == 0 else medium_count
track.medium, track.medium_index = medium_count, index_count
# Get `disctitle` from Discogs index tracks. Assume that an index track
# before the first track of each medium is a disc title.
for track in tracks:
if track.medium_index == 1:
if track.index in index_tracks:
disctitle = index_tracks[track.index]
else:
disctitle = None
track.disctitle = disctitle
return tracks
</code_context>
<issue_to_address>
**issue (code-quality):** We've found these issues:
- Replace unneeded comprehension with generator ([`comprehension-to-generator`](https://docs.sourcery.ai/Reference/Default-Rules/refactorings/comprehension-to-generator/))
- Swap positions of nested conditionals ([`swap-nested-ifs`](https://docs.sourcery.ai/Reference/Default-Rules/refactorings/swap-nested-ifs/))
- Hoist nested repeated code outside conditional statements ([`hoist-similar-statement-from-if`](https://docs.sourcery.ai/Reference/Default-Rules/refactorings/hoist-similar-statement-from-if/))
- Simplify dictionary access using default get ([`default-get`](https://docs.sourcery.ai/Reference/Default-Rules/refactorings/default-get/))
- Replace if statement with if expression ([`assign-if-exp`](https://docs.sourcery.ai/Reference/Default-Rules/refactorings/assign-if-exp/))
- Low code quality found in DiscogsPlugin.get\_tracks - 21% ([`low-code-quality`](https://docs.sourcery.ai/Reference/Default-Rules/comments/low-code-quality/))
<br/><details><summary>Explanation</summary>
The quality score for this function is below the quality threshold of 25%.
This score is a combination of the method length, cognitive complexity and working memory.
How can you solve this?
It might be worth refactoring this function to make it shorter and more readable.
- Reduce the function length by extracting pieces of functionality out into
their own functions. This is the most important thing you can do - ideally a
function should be less than 10 lines.
- Reduce nesting, perhaps by introducing guard clauses to return early.
- Ensure that variables are tightly scoped, so that code using related concepts
sits together within the function rather than being scattered.</details>
</issue_to_address>
### Comment 5
<location> `beetsplug/discogs.py:641` </location>
<code_context>
def get_track_info(
self, track, index, divisions, album_artist, album_artist_id
):
"""Returns a TrackInfo object for a discogs track."""
title = track["title"]
if self.config["index_tracks"]:
prefix = ", ".join(divisions)
if prefix:
title = f"{prefix}: {title}"
track_id = None
medium, medium_index, _ = self.get_track_index(track["position"])
artist, artist_id = self.get_artist(
track.get("artists", []), join_key="join"
)
# If no artist and artist is returned, set to match album artist
if not artist:
artist = album_artist
artist_id = album_artist_id
artist = self.strip_disambiguation(artist)
length = self.get_track_length(track["duration"])
# Add featured artists
extraartists = track.get("extraartists", [])
featured = [
artist["name"]
for artist in extraartists
if artist["role"].find("Featuring") != -1
]
if featured:
artist = f"{artist} feat. {', '.join(featured)}"
return TrackInfo(
title=title,
track_id=track_id,
artist=artist,
artist_id=artist_id,
length=length,
index=index,
medium=medium,
medium_index=medium_index,
)
</code_context>
<issue_to_address>
**issue (code-quality):** Use named expression to simplify assignment and conditional [×2] ([`use-named-expression`](https://docs.sourcery.ai/Reference/Default-Rules/refactorings/use-named-expression/))
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
snejus
reviewed
Sep 25, 2025
|
Thanks! Good catches with the comments, I'll update the tests and push a new commit later today. |
snejus
reviewed
Sep 27, 2025
|
thanks! |
snejus
approved these changes
Sep 27, 2025
3 tasks
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Description
Fixes #6038 - Appends featured artists in the extraartists field to the artist tag, similar to the MusicBrainz plugin. Works well with ftintitle for consistency as well.
To Do
May need adjustment for common artist delimiters used - but appears to match the MusicBrainz standard at the moment.
docs/to describe it.)docs/changelog.rstto the bottom of one of the lists near the top of the document.)