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 genre support using musicbrainz tags. #3781

Merged
merged 2 commits into from
Oct 28, 2020
Merged

Conversation

aereaux
Copy link
Contributor

@aereaux aereaux commented Oct 23, 2020

Description

Fixes #3053.

This used musicbrainz tags to populate the genre album tag. This is still a WIP, and it requires this PR (alastair/python-musicbrainzngs#266). Let me know if anyone has any ideas on how this could work better. Currently I just take the most-voted-for genre tag, breaking ties arbitrarily.

To Do

  • Documentation. (If you've add a new command-line flag, for example, find the appropriate page under docs/ to describe it.)
  • Changelog. (Add an entry to docs/changelog.rst near the top of the document.)
  • Tests. (Encouraged but not strictly required.)

@sampsyo
Copy link
Member

sampsyo commented Oct 24, 2020

Great! This seems like the right direction. Just choosing the most popular genre seems right to me, although an alternative would be to take all the genres and join them (as the lastgenre plugin can optionally do, when configured appropriately).

@aereaux
Copy link
Contributor Author

aereaux commented Oct 24, 2020

Yeah, part of posting this PR early is to see what would be the best interface for this. I mostly want it so that I can sort things in my library by genre (and in particular listing classical things by composer and work, and everything else by album). It seems like the best solution would be a multi-valued tag, but it sounds like that's not supported yet.

One difficulty with my mentioned use-case is releases such as this: https://musicbrainz.org/release/3f4ef072-37fe-4759-8a2d-7e6cb61cfafd . I think lastgenre uses a tag hierarchy to deal with this sort of thing.

@aereaux
Copy link
Contributor Author

aereaux commented Oct 26, 2020

OK, so I've changed this to add all tagged genres as a semicolon separated list. Might need some testing on more popular collections, not sure how well-tagged some albums are. I'm solving my use-case by using path queries on the genre tag, which works quite well. Anyways, seems ready for review to me.

@aereaux aereaux marked this pull request as ready for review October 26, 2020 15:25
@aereaux aereaux force-pushed the add_genres branch 2 times, most recently from a168f44 to 752fb5e Compare October 26, 2020 15:28
@sampsyo
Copy link
Member

sampsyo commented Oct 27, 2020

Super cool!! Just to clarify, do we need to wait for the new version of python-musicbrainz-ngs? I think the answer is "no" (this will just do nothing when genres are missing), but I wanted to double-check.

@aereaux
Copy link
Contributor Author

aereaux commented Oct 27, 2020

Unfortunately, with v0.7.1 of mb-ngs I believe it should error because the changes to add genres to the list of supported includes is only in master so far (alastair/python-musicbrainzngs#263). Not sure what the best solution is.

@sampsyo
Copy link
Member

sampsyo commented Oct 27, 2020

Got it. Maybe we should add an explicit check that asks whether 'genres' in musicbrainzngs.TAG_INCLUDES or similar, and only then request genres?

@aereaux
Copy link
Contributor Author

aereaux commented Oct 27, 2020

OK, added. Thanks for reviewing quickly.

@sampsyo
Copy link
Member

sampsyo commented Oct 28, 2020

Cool; thanks!! Is there any chance you could confirm, just to be sure we don't do something dumb, that this version of the code does not break on the currently-released version of the library?

It also might be nice to mention in the changelog that getting this new support depends on a future release of the relevant library.

@aereaux
Copy link
Contributor Author

aereaux commented Oct 28, 2020

Yep, just reinstalled the non-git version and verified it. (Although I would have expected it to get rid of my current genre information, but I guess it keeps it.)

Copy link
Member

@sampsyo sampsyo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looking good; thanks for following up. Looking at the code, I just found one simplification/removal of redundancy to suggest. If that looks good, then I think this is ready to merge.

beets/autotag/mb.py Outdated Show resolved Hide resolved
Co-authored-by: Adrian Sampson <[email protected]>
@sampsyo sampsyo merged commit 072e1df into beetbox:master Oct 28, 2020
@mtrolley
Copy link
Contributor

@aereaux Reading the comments on this PR am I correct in understanding that this will always add every genre that anyone has tagged a release with? Picard has really nice genre support in that it will let me optionally use only the genre's I've added to MB releases myself, it'll fall back to album artist's genre if the individual release or release group doesn't have a genre, and it lets you set a maximum number of genres to use. Will you consider adding support for options like that?

Also, looking at the code I don't see that you can disable genre adding in Beets, which I assume means this could overwrite genre tags at are already in the files. I personally wouldn't want that, so it would be nice to have this as an opt-in option until it's configurable.

@sampsyo
Copy link
Member

sampsyo commented Oct 28, 2020

Aha, you know what, that is a good point—at the very least, it should be optional. Could you please add a config option, @aereaux? The single-user genre and fallback support sounds nice too but maybe we should make a feature request ticket for that to track it.

@aereaux
Copy link
Contributor Author

aereaux commented Oct 28, 2020

@trolley Correct, this will add all genres that have been tagged. My use-case has been satisfied, so I am less motivated to implement additional functionality, but if you have a specific specification for how you want it to work, I could probably work on implementing it.

Currently tag overwriting shouldn't be an issue unless someone is testing it out with my musicbrainzngs PR, but I can create a PR for a config option once I get a chance. (I don't think I've interacted with the config system before, so I need to figure that out.)

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.

Get genres from MusicBrainz
3 participants