Add mbpseudo plugin for pseudo-release proposals#5888
Add mbpseudo plugin for pseudo-release proposals#5888semohr merged 13 commits intobeetbox:masterfrom
Conversation
|
Thank you for the PR! The changelog has not been updated, so here is a friendly reminder to check if you need to add an entry. |
Codecov Report❌ Patch coverage is
🚀 New features to boost your workflow:
|
6bae336 to
41487b3
Compare
fc81dc8 to
d864a24
Compare
a7df94f to
1fb8acf
Compare
2629a71 to
157f187
Compare
157f187 to
2681a5f
Compare
2681a5f to
e37c711
Compare
e37c711 to
f79eb11
Compare
|
I was wondering if it woudn't be easier to just extend the default |
|
@semohr I think that would be fine, I just didn't want to start by modifying the core plugin. I'm not sure how to go about the source weights though, would the distance calculation need to be modified to consider script somehow? Or how could the user nudge the proposals toward the pseudo releases? |
Instead, I was thinking along the lines of extending it, while keeping the logic isolated in its own plugin/file. Something like: from beetsplug.musicbrainz import MusicbrainzPlugin
class MBPseudoPlugin(MusicbrainzPlugin):
passThat way, the original plugin remains untouched, and this extension can encapsulate the new behavior.
I don’t think special handling is strictly necessary here, but I’m not 100% certain. You might want to take a look at the functions in |
|
Ah I see, I imagine that should be fine. I'm away from my pc for some time, but I'll take a look at that option. |
74072ef to
94e8f19
Compare
|
@semohr I changed the implementation as suggested and I think it looks good. The config I mentioned in the original post still applies. I've been validating with this release and I do get the pseudo-release as the main proposal. If this is ok, I'll add some tests and docs. |
94e8f19 to
4eced6a
Compare
There was a problem hiding this comment.
This looks pretty amazing and way cleaner now! Nicely done.
Some more things before we merge this. I think you wanted to do this either way but the plugin needs some documentation. Also while not required tests would be highly appreciated.
Another minor thing: We are trying to not use Optional[...] instead please use ... | None (this might need a from __future__ import annotations ). Also if you want to continue maintaining this plugin we would highly appreciate if you could add you handle to the CODEOWNERS file. This way you get updates if any PRs try to apply changes to the plugin (totally optional).
|
Lovely, thank you for looking into this, @asardaes, and @semohr for suggesting |
|
Something like in my custom base class could be implemented in MetadataSourcePlugin, so plugins can like mine easily define fields in their namespace which is inferred from |
|
Honestly, I don't care about having multiple artists, titles, etc. I just think it's useful for id fields. Everything else can be inferred from the ids by reimporting. It would also fix multiple plugins trying to override the same album or item when running |
|
@miblodelcarpio I added the option to only add custom tags. Since beets already supports custom path configurations, it seems this would accomplish what you wanted: paths:
default: %ifdef{album_artist_transl, $album_artist_transl, $albumartist}/%ifdef{album_transl, $album_transl, $album}%aunique{}/$track%ifdef{title_transl, $title_transl, $title} |
59df605 to
0443313
Compare
|
@asardaes Oh wow, thank you so much! Really looking forward to giving this a spin. Coupled with the conditional logic happening in item_fields:
artist: %ifdef{artist_transl, $artist_transl, $artist}
album: %ifdef{album_transl, $album_transl, $album}
title: %ifdef{title_transl, $title_transl, $title}While |
0443313 to
dbd29e3
Compare
|
@semohr I don't have further changes if you agree with the latest additions. |
dbd29e3 to
92998cd
Compare
|
In my head this is already approved! I'm waiting for some free time to have one closer look and merge this. I haven't forgotten you, please be patient for a bit longer 🙏 |
92998cd to
0899374
Compare
This reverts commit f3ddda3.
0899374 to
c087851
Compare
Description
Hi there, I wanted to implement this for my foreign music and I would be glad to get some feedback from the maintainers. I will address all the TODOs, but first I'd like to know if this fits.
The auto-tagger can already resolve official releases from pseudo-releases, but this only happens if the user provides search IDs manually. Similarly,
import.languagescan be used to search for artist aliases (and that does happen automatically), but that doesn't apply to album and track names.This plugin proactively searches for pseudo-releases during import and adds them as candidates. Since it also depends on MusicBrainz, there are some special considerations for the default logic (which is now a plugin as well). However, at the very least it expects a list of desired names of scripts in the configuration, for example:
It will use that to search for pseudo-releases that match some of the desired scripts, but will only do so if the input tracks match against an official release that is not in one of the desired scripts.
Standalone Usage
This would be the recommended approach, which involves disabling the
musicbrainzplugin. Thembpseudoplugin will manually delegate the initial search to it. Since the data source of official releases will still match MusicBrainz, weights are still relevant:A setup like that would ensure that the pseudo-releases have slightly more preference when choosing the final proposal.
Combined Usage
I initially thought it would be important to coexist with the
musicbrainzplugin when it's enabled, and reuse as much of its data as possible to avoid redundant calls to the MusicBrainz API. I have the impression this is not really important in the end, and maybe things could be simplified if we decide that both plugins shouldn't coexist.As it is right now, using both plugins at the same time would still work, but it'll only avoid redundancy if
musicbrainzemits its candidates beforembpseudo,which is why I modified the plugin-loading logic slightly to guarantee ordering. I'm not sure if you think this could be an issue, but I think themusicbrainzplugin is also used by other plugins and I can imagine it's good to guarantee the order that is declared in the configuration?If the above is fulfilled, the
mbpseudoplugin will use listeners to intercept data emitted by themusicbrainzplugin and check if any of them have pseudo-releases that might be desirable.To Do