-
Notifications
You must be signed in to change notification settings - Fork 1.8k
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 work, work-disambig and work_id tags #3272
Conversation
That should nail all changes to the core. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice; thanks for splitting this up! As a warning, however, we're in the midst of moving MediaFile to a separate repository—we may ask you to move this PR there.
So should I wait for my PR until MediaFile is done moving? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice; thanks again! Here are a few more small comments.
The MediaFile move is essentially complete, we're just delaying the final step to make sure we don't conflict with relevant open PRs like this one. It's currently identical to the copy of mediafile in beets, except that we added the MB work ID tag there already. Besides the MB work ID, I'm not sure how related your other changes to MediaFile are to the main change here. In any case it would be ideal if you could submit those as a separate PR against beetbox/mediafile and remove all of the MediaFile changes from this PR. |
The changes to mediafile have been reverted. Last thing to do before merge as far as I see it is to update the changelog, are there any subtleties regarding that? (AppVeyor fails because: |
whoops closed accidentally... |
Actually, I noticed that very few fields use the disambiguation, I wonder why? |
It would be really nice if we had multi-valued fields (#505) but that seems like it's a long way from happening. MediaFile already supports list-type fields but I'm not very familiar with how that works. I feel like the ad-hoc serialisation of the lists in this PR might be a bit premature. Could we simplify things by making the assumption that each track has only a single work? I know that it won't solve some of the more complicated cases like classical music, but there are other multi-valued fields like |
Multi-valued fields would be perfect, do you have an idea on how far this is advanced? The whole point of this is to handle classical music, for other types of music you don't need the work (not that I know). Maybe we can keep only one disambiguation, but I would keep all the works. |
Is it possible to store dicts in the database? This could solve our problems, and we would just need to handle the conversion from a dict to a tag. An artist/composer/... would have a dict with his name, sort name, credited name and disambiguation and the tag would be constructed from a list of dicts (a bit like it is done in musicbrainz-ngs. |
Multi-valued tags are sort of a long-term proposition—there are a lot of details to be worked out, so let's not depend on that happening to get this done. I think we should follow the behavior of other fields that also could be multi-valued, such as artist, and just make do with either a joined string or picking the first. Going with the simplest possible solution for now, seeing how it pans out, and iterating over time seems like a good strategy. |
Yep exactly, my suggestion was to only keep the first work for now because otherwise when we eventually come to solve multi-value tags properly we'll have to deal with different fields doing their own list serialisation. I know that we'd like multiple works per track eventually, but from what I understood from the parentwork plugin that motivated this PR you only need to pick up a single work and then recurse up to a root work. I might have misunderstood something but it doesn't seem like you need more than one work per track to do that. Also, in this PR you've written the field descriptions as though they are all single-valued fields. |
Sounds fine to me! Commit incoming. |
Thanks for your work on this! I think this looks good to go now. Multi-value tags is high on my list of tasks to tackle, but like Adrian said there's still quite a bit of design to sort out it, so it might not happen for a while yet. |
Merged with changelog! Thanks for your continued attention to this, @dosoe! |
add work, work-disambig and work_id tags
implements the first part of #2580 : changes to the core by adding work, work-disambig and work_id tags. I didn't change the changelog, I don't really get how it works now. @sampsyo , @arcresu , how do I update the changelog now?