Ability to optionally disable the musicbrainz metadata source #4319
Conversation
There was a problem hiding this comment.
Awesome; thanks for getting this started! I actually think that it might be a little cleaner to handle this disabling in the beets.autotag.hooks module, which is the "dispatcher" that aggregates results from multiple sources. For example, this is the line that asks MB for matches:
Line 615 in 3f6da68
Maybe it would make a little more sense to skip those calls there, rather than having them all return None?
beets/autotag/mb.py
Outdated
| @@ -482,6 +482,9 @@ def match_album(artist, album, tracks=None, extra_tags=None): | |||
| The query consists of an artist name, an album name, and, | |||
| optionally, a number of tracks on the album and any other extra tags. | |||
| """ | |||
| if not config["musicbrainz"]["enabled"].get(bool): | |||
There was a problem hiding this comment.
The .get(bool) bit here is actually unnecessary; using this in an if does that automatically.
docs/reference/config.rst
Outdated
| @@ -716,6 +716,39 @@ Default: ``{}`` (empty). | |||
| MusicBrainz Options | |||
| ------------------- | |||
|
|
|||
| Default configuration:: | |||
There was a problem hiding this comment.
Just for my own sanity, could we keep this PR's docs changes focused on the new option? Some refactoring, reordering, and adding examples would be nice too, but it will be easier to think about in a separate PR. Thank you!
There was a problem hiding this comment.
Of course, I'll move them out to another branch!
|
I do agree, will add the logic to |
|
It did end up being cleaner once the error handling logic got abstracted away. |
beets/autotag/hooks.py
Outdated
| if artist and album: | ||
| try: | ||
| yield from mb.match_album(artist, album, len(items), | ||
| extra_tags) | ||
| except mb.MusicBrainzAPIError as exc: | ||
| exc.log(log) | ||
| yield from handle_exc(mb.match_album, artist, *common_args) |
There was a problem hiding this comment.
In my opinion, this pattern would be much more readable as
if artist and album and config["musicbrainz"]["enabled"]:
yield from handle_exc(mb.match_album, artist, *common_args)with the check for MB being enabled removed from handle_exc. This would separate exception handling and checking whether to query musicbrainz somewhat more cleanly.
If not going this way, renaming handle_exc -> invoke_mb or similar would also help.
There was a problem hiding this comment.
Good point, @wisp3rwind. I've just renamed the function to invoke_mb
beets/autotag/hooks.py
Outdated
| if not config["musicbrainz"]["enabled"]: | ||
| return () |
There was a problem hiding this comment.
I do sort of like @wisp3rwind's suggestion to consider moving this out of the invoke_mb utility—it might make it a little clearer what's going on!
There was a problem hiding this comment.
I think I'm also fine with the current version (renamed to invoke_mb); I was mostly concerned about handle_exc being actively misleading since it does more than handling exceptions. So, from my side, I'd say it's up to you @snejus which variant you prefer.
| try: | ||
| return call_func(*args) | ||
| except mb.MusicBrainzAPIError as exc: | ||
| exc.log(log) |
There was a problem hiding this comment.
I think this might cause a crash when an exception occurs. Namely, it seems like this function needs to return an iterable, right? So I think this exception handler should probably return () or similar…
There was a problem hiding this comment.
Nice catch! Yes, I'd suspect the same.
There was a problem hiding this comment.
Well caught! Fixed in e1ffadb
beets/autotag/hooks.py
Outdated
| @@ -609,25 +619,17 @@ def album_candidates(items, artist, album, va_likely, extra_tags): | |||
| constrain the search. | |||
| """ | |||
|
|
|||
| common_args = [album, len(items), extra_tags] | |||
There was a problem hiding this comment.
I know this might be bikeshedding, but I actually think the repetition (passing the same set of arguments to the two calls to mb.match_album) is a little more intelligible.
There was a problem hiding this comment.
True. I must admit I sometimes treat the maximum line length a bit too religiously and adjust the code accordingly ... in unconventional ways 😆
beets/autotag/hooks.py
Outdated
| @@ -609,25 +619,17 @@ def album_candidates(items, artist, album, va_likely, extra_tags): | |||
| constrain the search. | |||
| """ | |||
|
|
|||
| common_args = [album, len(items), extra_tags] | |||
| # Base candidates if we have album and artist to match. | |||
| if artist and album: | |||
There was a problem hiding this comment.
This stanza and the next could share a single if config["musicbrainz"]["enabled"]:.
docs/reference/config.rst
Outdated
| .. _musicbrainz.enabled: | ||
|
|
||
| enabled | ||
| ~~~~~~~ | ||
|
|
||
| This option allows you to disable using MusicBrainz as a metadata source. This applies | ||
| if you use plugins that fetch data from alternative sources and should make the import | ||
| process quicker. | ||
|
|
||
| Default: ``yes``. |
There was a problem hiding this comment.
I think this documentation stuff might be out of order—it makes the "top-level" description below appear to be part of the "enabled" subsection. I think this should appear among the other subsections below (which start with "searchlimit").
There was a problem hiding this comment.
Agree - have moved it below.
Description
Fixes #400.
Related to #2686
As I mentioned in #400 (comment), this adds a patch that allows to disable the MusicBrainz metadata source during the import process.
I had a little go at making the
MusicBrainz Optionssection in the docs a little bit more consistent, and copied over the entire default config section for clarity.To Do
docs/to describe it.)docs/changelog.rstnear the top of the document.)