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

Discogs plugin replacing Feat. or Ft. with a comma #4401

Closed
kennyboy1978 opened this issue Jul 6, 2022 · 15 comments · Fixed by #4515
Closed

Discogs plugin replacing Feat. or Ft. with a comma #4401

kennyboy1978 opened this issue Jul 6, 2022 · 15 comments · Fixed by #4515
Labels
needinfo We need more details or follow-up from the filer before this can be tagged "bug" or "feature."

Comments

@kennyboy1978
Copy link

I try and use the Discogs plugin for my collection as I think it is more accurate than MusicBrainz. I've recently noticed that if Feat., Ft. or Vs. appears in the artist name on Discogs, it is replaced by a comma in between artists.

https://www.discogs.com/release/21846394-Vinylgroover-Feat-MC-Freestyle-Bassface

The output I want is:

Vinylgroover Feat. Dionne - Got Me Burning

But instead it's written as:

Vinylgroover, Dionne - Got Me Burning

Similarly:

https://www.discogs.com/release/22025074-Neophyte-Ft-Alee-Original-Gangster

Neophyte Ft. Alee - Original Gangster

Is written as:

Neophyte, Alee - Original Gangster

https://www.discogs.com/release/1694463-Scott-Brown-Vs-DJ-Rab-S-Now-Is-The-Time

Scott Brown Vs DJ Rab S - Now Is The Time

Scott Brown, DJ Rab S - Now Is The Time

Using Musicbrainz however, writes the format that I prefer. This would point to the way the plugin pulls the information, rather than something in my config file?

@RollingStar
Copy link
Collaborator

RollingStar commented Jul 6, 2022

How I would diagnose this:

I know plugins are stored in beetsplug:

https://github.com/beetbox/beets/tree/master/beetsplug

I see discogs.py ( I can also search for 'discogs' if i dont know the name of the plugin)

https://github.com/beetbox/beets/blob/master/beetsplug/discogs.py

I search "feat" - no results
"ft" - nothing

My next step would be to see how the discogs plugin is getting info from discogs. possibly with an "API" call. for musicbrainz (For example) you can look at what the program is seeing- ex.

https://musicbrainz.org/doc/MusicBrainz_API/Examples

edit:

Looks like it uses discogs_client, an outside library. That's as far as I'm taking this but others are welcome to continue the research.

def get_album_info(self, result):

You could also try increasing the logging level of python/beets/discogs client to the level "debug". I don't know offhand how to do that.

@sampsyo sampsyo added the needinfo We need more details or follow-up from the filer before this can be tagged "bug" or "feature." label Jul 7, 2022
@sampsyo
Copy link
Member

sampsyo commented Jul 7, 2022

This Discogs API, at least in the way we're using it, seems to return a list of artist names (e.g., Vinylgroover and Dionne, separately instead of a single string Vinylgroover Feat. Dionne). They are joined together into a single string here:

beets/beetsplug/discogs.py

Lines 568 to 570 in 472c3ab

artist, artist_id = MetadataSourcePlugin.get_artist(
track.get('artists', [])
)

It's possible Discogs also reports the single-string version of multi-artist music. If so, then we should probably use that! But it would require verifying that this information is actually available somewhere via the API…

@stale
Copy link

stale bot commented Sep 24, 2022

Is this still relevant? If so, what is blocking it? Is there anything you can do to help move it forward?

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

@JOJ0
Copy link
Member

JOJ0 commented Sep 25, 2022

If we look at the plain api response of aforementioned release: https://api.discogs.com/releases/21846394,
we see that the first artist contains "join": "Feat", the second artist does not contain that:

"artists": [
    {"name": "Vinylgroover", "anv": "", "join": "Feat", "role": "", "tracks": "", "id": 11433, "resource_url": "https://api.discogs.com/artists/11433"},
    {"name": "MC Freestyle", "anv": "", "join": "", "role": "", "tracks": "", "id": 336113, "resource_url": "https://api.discogs.com/artists/336113"}
],

In python3-discogs-client we could access the "join" information like this already and make use of it in beets:

>>> import discogs_client; d = discogs_client.Client('my_user_agent/1.0', user_token='REDACTED')
>>> release = d.release(21846394)
>>> print(release.artists[0].data["join"])
Feat

We also see that there is a separate object in the response that has it joined together already:

"artists_sort": "Vinylgroover Feat MC Freestyle",

I openend a pull request to implement this into python3-discogs-client: joalla/discogs_client#117,
once merged we could access it like this:

>>> import discogs_client; d = discogs_client.Client('my_user_agent/1.0', user_token='REDACTED')
>>> release = d.release(21846394)
>>> print(release.artists_sort)
Vinylgroover Feat MC Freestyle
>>> 

@stale stale bot removed the stale label Sep 25, 2022
@JOJ0
Copy link
Member

JOJ0 commented Sep 25, 2022

@sampsyo do you agree that the best option would be to use the new p3dc attribute artists_sort and let the beets user configure whether artist names should simply be joined using , as it is now, or the artits_sort field should be used. This would also require to increase the version dependance to p3dc in beets' setup.py (I'm sure we'll merge and release this tiny new feature in a jiff). I see beets uses extras_require in setup.py but without a version of python3-discogs-client. In any case this requirement should be noted in beets docs to "Using Discogs as the source plugin"

The alternative would be to use the "join" info from the artist data already as described above.

I would open a beets PR once we decide what's the best way to proceed. Thanks for your input in advance!

@sampsyo
Copy link
Member

sampsyo commented Oct 3, 2022

Aha, good question! I like the idea of using the join attributes that are available (even if that requires a version bump in the dependency). artist_sort is close to right, but I imagine it does other stuff to artist names to make them sortable—such as "Beatles, The".

@JOJ0
Copy link
Member

JOJ0 commented Oct 4, 2022

Ah good thinking, thanks! I double checked and yes indeed "Beatles, The" is what's containend in artists_sort for a good ol' Beatles release. Alright so then it's settled, I'll use the join attribute and note the version bump in the docs and in setup.py.

@JOJ0
Copy link
Member

JOJ0 commented Oct 7, 2022

Fun fact @sampsyo, the original PR implementing a first version of Discogs source plugin, already respected the join field in the Discogs API result:

https://github.com/beetbox/beets/pull/283/files#diff-c9f257e23ad1ebab4dafc681c7e0a22d3f7432eb8664e06cd7cf84ce5fc426d9R114-R115

At some point the get_artist method was moved out into a staticmethod in the MetadataSourcePlugin abstract class, which doesn't have this functionality anymore - it always returns a comma separated string of artists. I suppose that applies to other MetadataSourcePlugins as well, eg. I tested with the Spotify plugin: When selecting a release found on there, it faces the same "problem": Artists are combinend into a string like eg. "Artist1, Artist2". BTW FYI @arsaboo

I'm wondering if this functionality should be added to this "core" method again or if it is very Discogs specific and should be worked around in the Discogs plugin itself? An optional argument perhaps called 'join_key` could be added:

beets/beets/plugins.py

Lines 658 to 691 in 472c3ab

def get_artist(artists, id_key='id', name_key='name'):
"""Returns an artist string (all artists) and an artist_id (the main
artist) for a list of artist object dicts.
For each artist, this function moves articles (such as 'a', 'an',
and 'the') to the front and strips trailing disambiguation numbers. It
returns a tuple containing the comma-separated string of all
normalized artists and the ``id`` of the main/first artist.
:param artists: Iterable of artist dicts or lists returned by API.
:type artists: list[dict] or list[list]
:param id_key: Key or index corresponding to the value of ``id`` for
the main/first artist. Defaults to 'id'.
:type id_key: str or int
:param name_key: Key or index corresponding to values of names
to concatenate for the artist string (containing all artists).
Defaults to 'name'.
:type name_key: str or int
:return: Normalized artist string.
:rtype: str
"""
artist_id = None
artist_names = []
for artist in artists:
if not artist_id:
artist_id = artist[id_key]
name = artist[name_key]
# Strip disambiguation number.
name = re.sub(r' \(\d+\)$', '', name)
# Move articles to the front.
name = re.sub(r'^(.*?), (a|an|the)$', r'\2 \1', name, flags=re.I)
artist_names.append(name)
artist = ', '.join(artist_names).replace(' ,', ',') or None
return artist, artist_id

Update: This is where respecting join got lost back when MetadataSourcePlugin was introduced and Spotify, Deezer and Discogs plugins where refactored, have a look at the diff of discogs.py :-) https://github.com/beetbox/beets/pull/3371/files

@JOJ0
Copy link
Member

JOJ0 commented Oct 7, 2022

The alternative, in case we'd like to keep MetadataSourcePlugin.get_artist() as-is, would be to do something like this within the Discogs plugin:

        # Why not use artists_sort, get_artist handles moving "the" to front!
        artist, artist_id = MetadataSourcePlugin.get_artist(
            [{'name': result.artists_sort, 'id': result.artists[0].id}]
        )
  • MetadataSourcePlugin.get_artist() always returns the very first artist_id from the passed list anyway
  • It handles moving a|an|the to the front of the string
  • This could also be implemented by using the data attribute of python3-discogs-client object, in that case not even a version bump would be required.

But still I think, since stuff was moved out of plugins and generalized and things where forgotten, implementing into the "core" logic seems to be a good idea.

@JOJ0
Copy link
Member

JOJ0 commented Oct 7, 2022

Example import with last "workaround" solution:

(beets)    ~/git/beets   fix_discogs_multi_artist ●  beet import ~/Sync/beets_test_music/DJ\ Marky\ -\ LK

/home/jojo/Sync/beets_test_music/DJ Marky - LK (2 items)
Finding tags for album "DJ Marky & XRS, Stamina MC - LK 'Carolina Carol Bela'".
Candidates:
1. DJ Marky & XRS Featuring Stamina MC - LK 'Carolina Carol Bela' (71.8%) (id, artist, tracks) (Discogs, Vinyl, 2002, UK, V Recordings, V035)
...
...

You see this was previously tagged with the old behaviour - artists simply connected with ", " - and now is a single string.

@RollingStar
Copy link
Collaborator

Note: Musicbrainz supplies join keys iirc. Could be a useful crossreference on a search.

JOJ0 added a commit to JOJ0/beets that referenced this issue Oct 10, 2022
JOJ0 added a commit to JOJ0/beets that referenced this issue Oct 30, 2022
@JOJ0
Copy link
Member

JOJ0 commented Nov 3, 2022

By any chance @RollingStar you a maintainer and want to review my PR?

@RollingStar
Copy link
Collaborator

Sorry, not this time. I end up playing in people's sandboxes but I don't know how all the beets code works.

@JOJ0
Copy link
Member

JOJ0 commented Dec 23, 2022

@kennyboy1978 by any chance is it possible for you to test the new feature in the PR you find linked above. It might help getting this fix merged. I can help with installing beets from that branch, in case that's desired. Hope that helps!

@kennyboy1978
Copy link
Author

Sorry for not replying sooner.

I stumbled my way through installation and this works as expected.

Artists with "Feat, Ft, Vs and & " are all picked up now.

Thank you very much!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
needinfo We need more details or follow-up from the filer before this can be tagged "bug" or "feature."
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants