Add support for artists and albumartists multi-valued tags #4743
Conversation
| self.tracks = tracks | ||
| self.asin = asin | ||
| self.albumtype = albumtype | ||
| self.albumtypes = albumtypes or [] |
There was a problem hiding this comment.
This was missing from the other PR
|
Oh my God... this would be amazing. Can you add basic docs for someone to test this out? I'll test this out and report findings. |
I haven't tested this yet aside from unit tests. I too will give it a spin when I get more free time. |
|
I imported this album on my branch and verified that As for testing docs @arsaboo , multi tags for artists/albumartists should work out of the box - no additional configuration needed. I'll add some more unit tests to make sure id3v23 works as expected |
beets/autotag/mb.py
Outdated
| artist = recording['artist-credit'][0]['artist'] | ||
| info.artist_id = artist['id'] | ||
| info.artists, info.artists_sort, info.artists_credits = \ | ||
| _multi_artist_credit(recording['artist-credit'], include_join_phrase=False) |
There was a problem hiding this comment.
might be worth rewriting _multi_artist_credit and _flatten_credit_artist to only scan the artist-credit once. Currently, it's doing two passes
|
Any thoughts on this @sampsyo ? Would love some feedback on this - and sorry for being impatient 😅 |
There was a problem hiding this comment.
Wow!!! This is incredibly great work—many commendations, @jmbannon. I am super impressed by how simple these changes ended up being, especially given how low the bug number is for #505. 😃 I'm sure we could find an even more ambitious, complex way to approach this, but this seems like a very reasonable way to get most of the benefit without changing everything all at once.
I had a couple of very tiny comments within, but if this seems to work for everybody and passes CI, I think it's basically good to go. Again, huge thanks to @jmbannon for tackling a seemingly-intractable problem with aplomb.
| @@ -74,15 +74,19 @@ def __init__( | |||
| album_id: Optional[str] = None, | |||
| artist: Optional[str] = None, | |||
| artist_id: Optional[str] = None, | |||
| artists: Optional[List[str]] = None, | |||
| artists_ids: Optional[List[str]] = None, | |||
There was a problem hiding this comment.
FWIW, we've been gradually moving away from listing each field exhaustively in AlbumInfo/TrackInfo and instead just relying on **kwargs to convey all the information we need. But no big deal if it's easier to grok with these fields listed explicitly here.
There was a problem hiding this comment.
As boiler-plate as it is, I prefer it lol. For me, it helps to know which native field types are expected
| ) | ||
|
|
||
|
|
||
| def _flatten_artist_credit(credit: List[Dict]) -> Tuple[str, str, str]: |
There was a problem hiding this comment.
Really nice work on this refactoring. Clever and also simple!
I too was humbled by how simple this was. Goes to show I'm standing on the shoulders of giants One test failed because python <=3.8 and/or sqlite doesn't like the literal null terminator as a DB value (would only return the first element). Changed it to |
Dear @catduckgnaf, I want to be honest with you: Two days ago I was quite motivated to move on with this PR and help jmbannon move forward. But after our debate and in the end receiving the message I quoted above, I'm not at all motivated to move on with that topic. You are accusing me of several things: I'm wasting your time, I'm the reason that people walk away from the project and you are telling me "how it should be done". Do you really think that helps anyone? Well, we might have very different opinions on how Open Source projects should be handled, and certainly your opinions are valuable, BUT that does not mean you should throw them at us in that manner. Please go ahead and "politely" submit your suggestions into a fitting thread like that one: #4558 |
|
@JOJ0 I appreciate the time you volunteer to push beets forward. I'll apologize on behalf of @catduckgnaf , the bottom-line is we're both eager to see this PR through and it's been a bit frustrating waiting for a few months. But it's important to remember that open source doesn't owe us anything, including fast reviews/approvals/etc from folks like you who spend time maintaining great software for free. I'm still determined to do what needs to be done to get this feature pushed so everyone can enjoy its value. I hope we can continue collaborating - I would love to expand multi-tag functionality beyond this PR. That requires good terms and healthy collaboration, which I hope we can all commit to going forward. |
|
I too will try better going forward, even if it means going against my own personal best practices. Tomorrow I'll get this PR force pushed as a single commit like you asked |
No. Please dont rebase to one single commit. I never said I want a single commit. Please read my initial message again: #4743 (comment) Too summarize, I was talking about:
And now if you were really to tidy it up, I'd like to add:
But I have to admit that things changed in other parts of my life during the last days also and I have to finish something by the end of September, so probably October will be a better time to move on with "in detail / new territorry" beets things. So again, and also as written in that linked first comment, my offer was that, I could at least try to test the "usage" of the feature as a first step, to get a feeling from a user's perspective. What I currently can't offer, is reviewing the code in detail. Too much new territorry, I'm not the person for that. That's all I can offer but I'm interested how it works on the long run, but it will take time which I currently dont have. |
Thanks for the appreciation and the kind words and that you think about those things. Yes let's get back to healthy corporation, I'm sure we can do it :-) |
|
@JOJ0 I am sorry and wanted to apologize. |
Apology accepted. :-) We will do that! Soonish! |
i'm too impatient to wait for beetbox/beets#4743
|
I tried my best to go ahead and wrap my head around this feature. I read the comments history twice, the media file additions PR and did - at least - some playing around with it myself. Thanks for testing over there @arsaboo, that helped a lot to make me understand some things without having to look at code or play around myself initially. Thanks to you both @arsaboo and @jmbannon, just reading your conversation helped already!! Some playing around with a "two-artists-release" Database: File tags via beets, prior to updating mediafile (still 0.11.0): File tags via mutagen-inspect, prior to updating mediafile (still 0.11.0): File tags via mutagen-inspect, prior to updating mediafile (still 0.11.0): Updated mediafile to 0.12, writing tags: File tags beets now sees: File tags mutagen-inspect now sees: First of all, is what I'm seeing here the expected outcome? |
|
I'm very close to wanting to hit that merge button. Some questions come up:
Some random examples from somewhere around beets, how a descriptive commit messages could look like: I know that not everyone is as tidy / likes to write a lot of words as the authors above do - but at least quite a lot of our contributors do - I'm trying again to make this happen - if not - I'll give up ;-)
|
|
@JOJ0 Much thanks for all your time testing this out. Over the course of today, I'll try to respond to all of your questions from top-to-bottom |
|
@JOJ0 All your commands look good. In the database, it writes I'm not familiar with mutagen-inspect - I assume
See Line 283 in 4f58b03
The ones on the top-of my mind are:
I suggest we do these in a follow-up PR since this one is already pretty big
I really think we should squash merge this as a single-commit to master since it's adding a single feature with tests to beets. My concern regarding history was just for the PR/branch. Future reviewers can refer to this PR to get the history. And worst-case if we need to revert the commit, it will be seamless since it's packaged into a single one.
IMO I don't think this PR needs any additional docs added to the readthedocs - it's simply adding more musicbrainz fields (which aren't documented as-is), and there already is a notion of using id3v2.3 vs id3v2.4 tags here: https://beets.readthedocs.io/en/stable/reference/config.html?highlight=id3#id3v23
Nothing is in stone yet, but @sampsyo provided some good wisdom on how we should approach it. Again, I think it's okay to put this off for a future PR + reboot the discussion. Any multi-tag, regardless of id3v2.3 vs 2.4 has the same issue - it's not specific to this PR changeset. |
GitHub lets you change the commit message when you squash it. If you're okay with that approach, I can draft a message that you can use if/when you do the squash + merge |
Alright, then lets go that way and I'll hit that merge+squash button :-) |
|
@JOJ0 Sounds good! I'll get something drafted later today |
|
Draft message for the squash merge commit, feel free to change as you see fit: |
and fix wording to use "multi-valued tags" instead of "multi-tags".
JOJ0
changed the title
|
@jmbannon I did some nitpicking on the wording in docs, changelog and PR/issue naming. Sometimes we referred as multi tags, multi-tags, multi-valued tags, multiple-valued tags, and I decided that mutli-valued tags is the most descriptive one which still is not too longish. Also in code we have MULTI_VALUE_DSV for example which fits with that wording. Trying to make sure wording is easy to grasp for readers not "in to" the topic yet. HTH :-) I was thinking about adding an example about what you wrote in the docs "use regex" but didn't in the end since I trust that you will do that soon in your next PR, and maybe we have better options for searching then already :-) Looking forward to your next contributions, thanks so much for this awesome feature, sorry that it all took so long and speak soon :-) |
) Adds the following fields with id3v2.4 multi-valued tag support to autotag: - artists, artists_sort, artists_credit - albumartists, albumartists_sort, albumartists_credit - mb_artistids, mb_albumartistids MusicBrainz support to populate + write the above multi-valued tags by default. Can be toggled to use id3v2.3 or id3v2.4 tags via the existing beets configuration option `id3v23`. Big thanks to @JOJ0, @OxygenCobalt, @arsaboo for testing + @sampsyo for the initial code review .
|
I've been searching around for a way to split the new arrays, this is not working for me: Edit: just to report, this works for me: |
|
maybe @jmbannon can clarify what's the intended way of doing it |
|
While waiting I'm trying to import my music and noticed another issue with the rewrite plugin, it crashes beets if I try to alter albumartists with it. Looking at the backtrace, it tries to convert a list into lowercase. Just my guess but this change intoduced lists where previously there could only be strings? |
|
I'm using current I'm trying to provide a list of However, I keep receiving this issue And worst of all - if this happens when I try to reimport an album that already exists in the database ( @jmbannon have you encountered this by any chance? Or am I missing something in my setup? |
Description
#505
Piggy-backs on the work done in #4582 to add support for:
artistsartists_sort(mediafile support added in multi-valued tags for artists + albumartists sort and credits mediafile#69)artists_credit(mediafile support added)albumartistsalbumartists_sort(mediafile support added)albumartists_credit(mediafile support added)mb_artistidsmb_albumartistidsmulti-tags as part of the default behavior when fetching tags from musicbrainz. The ones marked accordingly require mediafile >=0.12.0 to write the tags to the file.
setup.pyis updated to reflect this.I think
albumtypeshas paved the way on how multi tags should be handled - using a delimiter and storing them in the database as a string, but writing them to tags as a multi (list).Some considerations with this approach are:
artists::Eric Clapton) since the db's literal value will beEric Clapton\␀B.B. King/for a delimiter, example above would writeEric Clapton/B.B. KingPotential follow-ups:
genre/genrestag like this (PR is up! Add support for multi-valued genre tag #4751)To Do
docs/to describe it.)docs/changelog.rstnear the top of the document.)