Save Discogs Release ID via MusicBrainz

[JOJ0]

Description

• Fixes #4220
• Enable fetching 'url-rels' from the musicbrainz release endpoint.
• A musicbrainz release might contain a link to a discogs release or a discogs master release.
• We prefer the regular release and save it to the beets library field discogs_albumid. as a fallback we save the master release id.
• For extraction of the release id from the discogs url, we use the methodfound in the discogs plugin.

To Do

• [x] The discogs plugin needs to be refactored to use the method outsideof the DiscogsPlugin class...
• [ ] Decide whether or not the extract_discogs_id_regex function should also accept master release urls or if that should rather be handled in a separate function.
• [ ] Documentation.
• [ ] Changelog.
• [ ] Tests.

[JOJ0]

Is it ok if I move the extract_release_id_regex method out of the DiscogsPlugin class but leave it in the module so others can import it from there? Or should I rather move it into the general beets.util module?

[JOJ0]

Is it ok if I move the extract_release_id_regex method out of the DiscogsPlugin class but leave it in the module so others can import it from there? Or should I rather move it into the general beets.util module?

Doesn't work like that! I was getting a circular import error with that approach thus I decided that moving to beets.util for the discogs extractor function is a possible option: https://github.com/beetbox/beets/pull/4474/commits/ce5d659a9f8e924c32ca35f4f86994a59c922c47

Dear maintainers, can you think of a better place or approach to make this helper method usable from within the discogs plugin but also from other plugins, or if my solution is ok like that. Thanks in advance!

[JOJ0]

@wisp3rwind since you were involved in the process of extending the discogs url to id extractor method a couple of months ago. I extended it with the short form url format that is used very often in the discogs relation links saved in musicbrainz releases (and also my go to move if I have a discogs release id at hand and want to quickly look it up). May I ask for an early review from you: I extended the regex and saved a new version on regex101: https://github.com/beetbox/beets/pull/4474/commits/693413146560fa7498e913e17a83a42f23e961be

[arsaboo]

This is pretty epic. I think it would be great to have this function accessible in other plugins. For example, I would love to be able to translate between MusicBrainz artist_id and Spotify artist_id.

[JOJ0]

Hi @sampsyo as recently discussed in #4537 I removed the "prevent ID overwrite" functionality and finished this PR. I'd kindly request a final review. I try to summarize what I think should be looked into in particular:

• As you've already approved, I moved the Discogs id extraction to the utils module.

• Some parts, in particular the actual regex strings of all the available metadata source plugins, had to move there as well.

• I went a step further and even invented a submodule in utils named id_extractors because I think it's much cleaner than putting more stuff into the already poluted utils module directly: https://github.com/beetbox/beets/pull/4474/files#diff-52a89c1045582fbed2d44321767c31a4b6e70c836ac234e60b2fcb26f64c4702

• The id extraction method in the MetadataSourcePlugin abstract class had to be refactored just a little. It is now a staticmethod and it's possible to use it from the outside of plugins: https://github.com/beetbox/beets/pull/4474/files#diff-165009439d41875b686d8cd7d4a1d698bcd33d53bf2d8748d13f3e9c59592c33

• The metadata source plugins required tiny refactoring as well: https://github.com/beetbox/beets/pull/4474/files#diff-0b73870a2e6c32c52594c22179e987e178f2bde3bbb2f1cd6546ab90723a4a41.

• The beatport plugin had some things built-in that's actually covered by the abstract class already. I refactored it, it's now streamlined with the other plugins that are using the abstract class.

• I decided to always include the additional url-rels data when musicbrainz is requested in the autotagger because I suppose it's not much of a performance impact - open for discussion if you think it's not a good idea: https://github.com/beetbox/beets/pull/4474/files#diff-ff51bca3f15a32bf8652a69e127b582652187bae2fd2fd7a1ccb58d0feeb97b2

• The reimport_metadata method in the importer required some "exclusion code" to prevent the overwriting of flexible attributes on reimports. I left a longish comment there to make it clear why this is there. https://github.com/beetbox/beets/pull/4474/files#diff-bb5be0a837ad397e52d11fc49be3bbbff18df98a6feae43208dd2231929f2fed

I hope that helps with reviewing, in particular with understanding the why's of some of my changes quicker!

[JOJ0]

I was wondering whether it would help maintainers (in that case most probably this is directed to @wisp3rwind and @sampsyo) if I split up this PR into 2 pieces? Human resources in the project are still rare I guess and I played with the thought that it might help to first submit a PR implementing only the "refactoring metadata ID extraction" part and then the actual feature. I think the former is a nice addition to beets on its own. It even fixes some things and most of all streamlines things and I think that adds to the project already. And certainly my hope is that after that would be merged, the actual feature I'd like to bring into beets would result in a slightly smaller and hopefully a little easier to review PR.

What do you think? Would that make sense? I have a few days off from work and could start working on it soon.

[wisp3rwind]

I was wondering whether it would help maintainers (in that case most probably this is directed to @wisp3rwind and @sampsyo) if I split up this PR into 2 pieces? Human resources in the project are still rare I guess and I played with the thought that it might help to first submit a PR implementing only the "refactoring metadata ID extraction" part and then the actual feature. I think the former is a nice addition to beets on its own. It even fixes some things and most of all streamlines things and I think that adds to the project already. And certainly my hope is that after that would be merged, the actual feature I'd like to bring into beets would result in a slightly smaller and hopefully a little easier to review PR.

What do you think? Would that make sense? I have a few days off from work and could start working on it soon.

Splitting things into small orthogonal PRs sounds like a good idea! (Even though, at first sight, this PR doesn't appear exactly gigantic.) That being said, don't rely on me personally reviewing this very soon: There's limited time I can and want to spend on beets; and one thing that I've been trying to do is focus reviews on parts of beets that I actually know about. The metadata sources are decidedly not in that scope, and there are already a few other important PRs that I really should get around to (for which I've been involved with already, in contrast to this PR); sorry!

[JOJ0]

Thanks a lot for an honest answer, letting me know what to expect from your angle. Appreciate it! Since I use this feature everyday anyway from my own branch I don't have any rush in getting this reviewed. I'll consider creating a PR-split but in the meantime I picked up other tasks related to my beets tinkering and dev. Thanks for letting me know that you think it is basically a good idea.

[JOJ0]

Changed this PR back to draft and "split out" the metadata ID extraction part into new PR https://github.com/beetbox/beets/pull/4633

[JOJ0]

Closing in favor of new PR #4220