Discogs: artist name variation support + artist credit support. #6050
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
…le featured credit.
Reviewer's GuideThis PR adds support for Discogs-provided artist name variations (ANVs) with configurable application to album artist, track artist, and artist credit tags, introduces a customizable featured artist string, and updates the Discogs plugin implementation, tests, and documentation accordingly. File-Level Changes
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 and they look great!
Prompt for AI Agents
Please address the comments from this code review:
## Individual Comments
### Comment 1
<location> `beetsplug/discogs.py:406-407` </location>
<code_context>
# Additional cleanups
# (various artists name, catalog number, media, disambiguation).
if va:
- artist = config["va_name"].as_str()
- else:
- artist = self.strip_disambiguation(artist)
+ album_artist, artist_credit = config["va_name"].as_str()
if catalogno == "none":
catalogno = None
</code_context>
<issue_to_address>
**issue (bug_risk):** Assigning config["va_name"].as_str() to two variables may cause issues.
Unpacking config["va_name"].as_str() into two variables will fail unless it returns a tuple or list. Assign the value to both variables explicitly or ensure the config returns a tuple.
</issue_to_address>
### Comment 2
<location> `beetsplug/discogs.py:695` </location>
<code_context>
- if featured:
- artist = f"{artist} feat. {', '.join(featured)}"
- artist = self.strip_disambiguation(artist)
+ if extraartists := track.get("extraartists", []):
+ featured_list = [
+ artist
</code_context>
<issue_to_address>
**suggestion:** Featured artist logic may not handle empty featured_list gracefully.
Check that calling get_artist with an empty list does not produce unwanted separators or malformed output.
</issue_to_address>
### Comment 3
<location> `test/plugins/test_discogs.py:456-465` </location>
<code_context>
[email protected](
</code_context>
<issue_to_address>
**suggestion (testing):** Consider adding edge case tests for missing or empty 'anv' fields.
Please include tests for cases where 'anv' is missing, empty, or None to verify correct fallback behavior.
Suggested implementation:
```python
@pytest.mark.parametrize(
"config_input,expected_output",
[
(
{
"track_artist_anv": False,
"album_artist_anv": False,
"artist_credit_anv": False,
},
{
"track_artist": "ARTIST Feat. PERFORMER",
),
# Edge case: 'anv' field missing
(
{
"track_artist_anv": True,
"album_artist_anv": True,
"artist_credit_anv": True,
# Simulate missing 'anv' by not providing it in the test data setup
},
{
"track_artist": "ARTIST Feat. PERFORMER", # Should fallback to default name
},
),
# Edge case: 'anv' field is empty string
(
{
"track_artist_anv": True,
"album_artist_anv": True,
"artist_credit_anv": True,
"anv": "",
},
{
"track_artist": "ARTIST Feat. PERFORMER", # Should fallback to default name
},
),
# Edge case: 'anv' field is None
(
{
"track_artist_anv": True,
"album_artist_anv": True,
"artist_credit_anv": True,
"anv": None,
},
{
"track_artist": "ARTIST Feat. PERFORMER", # Should fallback to default name
},
),
```
You may need to ensure that the test data setup in the test function correctly simulates the missing, empty, and None 'anv' cases. If the test function expects a specific structure for the input, you might need to adjust how the 'anv' field is injected or omitted in the test setup.
</issue_to_address>
### Comment 4
<location> `test/plugins/test_discogs.py:487-496` </location>
<code_context>
+ assert r.tracks[0].artist_credit == expected_output["track_artist_credit"]
+
+
+@patch("beetsplug.discogs.DiscogsPlugin.setup", Mock())
+def test_anv_album_artist():
+ """Test using artist name variations when the album artist
+ is the same as the track artist, but only the track artist
</code_context>
<issue_to_address>
**suggestion (testing):** Add a test for mixed ANV configuration (e.g., only artist_credit_anv enabled).
Please add a test with only 'artist_credit_anv' set to True and the others False to confirm correct behavior for this configuration.
</issue_to_address>
### Comment 5
<location> `test/plugins/test_discogs.py:565-571` </location>
<code_context>
+ config["discogs"]["track_artist_anv"] = True
+ config["discogs"]["artist_credit_anv"] = False
+ r = DiscogsPlugin().get_album_info(release)
+ assert r.artist == "ARTIST"
+ assert r.artist_credit == "ARTIST"
+ assert r.tracks[0].artist == "VARIATION"
+ assert r.tracks[0].artist_credit == "ARTIST"
+
+
</code_context>
<issue_to_address>
**suggestion (testing):** Consider adding a test for multiple artists with different ANV values.
Please add a test case with multiple artists, each having a distinct ANV, to verify correct handling and joining of artist names.
```suggestion
config["discogs"]["track_artist_anv"] = True
config["discogs"]["artist_credit_anv"] = False
r = DiscogsPlugin().get_album_info(release)
assert r.artist == "ARTIST"
assert r.artist_credit == "ARTIST"
assert r.tracks[0].artist == "VARIATION"
assert r.tracks[0].artist_credit == "ARTIST"
def test_multiple_artists_with_distinct_anv():
# Simulate a release with two artists, each with a different ANV
data = {
"title": "Test Album",
"artists": [
{"name": "Artist One", "anv": "A1"},
{"name": "Artist Two", "anv": "A2"},
],
"tracklist": [
{
"title": "Track 1",
"artists": [
{"name": "Artist One", "anv": "A1"},
{"name": "Artist Two", "anv": "A2"},
],
}
],
}
release = Bag(
data=data,
title=data["title"],
artists=[Bag(data=d) for d in data["artists"]],
)
config["discogs"]["album_artist_anv"] = True
config["discogs"]["track_artist_anv"] = True
config["discogs"]["artist_credit_anv"] = False
r = DiscogsPlugin().get_album_info(release)
# Check that the album artist is joined ANVs
assert r.artist == "A1, A2"
# Check that the album artist_credit is joined names
assert r.artist_credit == "Artist One, Artist Two"
# Check that the track artist is joined ANVs
assert r.tracks[0].artist == "A1, A2"
# Check that the track artist_credit is joined names
assert r.tracks[0].artist_credit == "Artist One, Artist Two"
```
</issue_to_address>
### Comment 6
<location> `beetsplug/discogs.py:472` </location>
<code_context>
def get_tracks(self, tracklist, album_artist_data):
"""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_data
)
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>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
semohr
reviewed
Sep 30, 2025
semohr
reviewed
Oct 1, 2025
semohr
reviewed
Oct 1, 2025
semohr
reviewed
Oct 1, 2025
semohr
reviewed
Oct 1, 2025
semohr
approved these changes
Oct 5, 2025
There was a problem hiding this comment.
Thanks for adding so many type hints! I only expected them for the functions you modified, so this was a nice surprise.
In general, using cast isn’t ideal, but I understand why you used it here, avoiding it would require a fair amount of refactoring. Maybe we can tackle that in a future PR 🙃
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
Implements #3354, updates #6040
The name variations provided by Discogs can now be used by the auto-tagger through 3 config options.
By default, the plugin will write the variation to the tag
artist_credit, but through three config options can also write itto the album artist tag, the track's artist tag, or any combination of the three.
This PR contains a small addition on #6040, letting the string used to join featured artists be customized.
The new configuration options available, and their defaults, are as follows:
To Do
Configuration options could be renamed, but I think they're decently self documenting at the moment.
docs/to describe it.)docs/changelog.rstto the bottom of one of the lists near the top of the document.)