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

Add discogs_labelid and discogs_artistid fields #3413

Merged
merged 2 commits into from Oct 26, 2019
Merged

Conversation

msil
Copy link
Contributor

@msil msil commented Oct 24, 2019

This patch adds some extra discogs fields. I see this as a step forward towards not having the discogs plugin pollute/overwrite the mb_albumid/etc fields.

@msil
Copy link
Contributor Author

msil commented Oct 24, 2019

I have added some new fields to mediafile so we can store these values in the tags:
beetbox/mediafile#22

@sampsyo
Copy link
Member

sampsyo commented Oct 24, 2019

Hi! This seems totally reasonable—storing the label and artist IDs seems useful. But for what it's worth, we don't actually need to add either of these to MediaFile to get started. They can just be stored in the database.

Any chance you could add a quick note to the changelog?

@msil
Copy link
Contributor Author

msil commented Oct 24, 2019

I've updated the changelog. Do you think it's worth it for me to start work on having the discogs plugin leave the mb_* fields alone and just touch the discogs_* fields going forward? I've had issues with some software (funkwhale for example) which are expecting proper MusicBrainz UUIDs in the mb_* tags.

@msil
Copy link
Contributor Author

msil commented Oct 24, 2019

This is part of the way to (hopefully) closing off #604. I'm also quite interested in looking at implementing something for the ideas discussed in #2866 unless anyone has any objections.

@sampsyo
Copy link
Member

sampsyo commented Oct 24, 2019

Looks good! We can merge this now, if you think it's ready.

On those two other issues:

  • Addressing Discogs (and other metadata sources) polluting mb_albumid #604 would be great! The next step, however, would be to change the core autotagger matching logic to allow empty MBID fields. It currently assumes it can use the ID fields to deduplicate redundant matches. Once that's done, it should be possible to remove the MBID "pollution" from the metadata plugins, including the Discogs one.
  • Addressing Importer: match and store metadata from multiple sources #2866 would also be cool but, as a warning, seems to me like a really big project. It will have to touch the importer UI and the core importer pipeline, both of which are pretty extensive and intricate piles of hacks. Now you know! But don't let that stop you from trying to dive right in. 😃

@msil
Copy link
Contributor Author

msil commented Oct 25, 2019

I'm happy for this to get merged now, thanks.

I'll start looking at the other two issues over the next few days. Indeed #2866 is a bit daunting but I'm up for the challenge :)

Thanks for the advice!

@sampsyo
Copy link
Member

sampsyo commented Oct 26, 2019

Awesome; thanks!!

@sampsyo sampsyo merged commit 1b187fb into beetbox:master Oct 26, 2019
@dbogdanov
Copy link
Contributor

This is missing discogs_releasegroupid. Add when available for consistency?

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.

None yet

3 participants