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 work, work-disambig and work_id tags #3272

Merged
merged 14 commits into from May 31, 2019
Merged

Conversation

dosoe
Copy link
Contributor

@dosoe dosoe commented May 25, 2019

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?

@dosoe
Copy link
Contributor Author

dosoe commented May 25, 2019

That should nail all changes to the core.

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.

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.

beets/autotag/mb.py Outdated Show resolved Hide resolved
beets/autotag/mb.py Outdated Show resolved Hide resolved
@dosoe
Copy link
Contributor Author

dosoe commented May 26, 2019

So should I wait for my PR until MediaFile is done moving?

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.

Nice; thanks again! Here are a few more small comments.

beets/library.py Outdated Show resolved Hide resolved
beets/autotag/mb.py Outdated Show resolved Hide resolved
beets/mediafile.py Outdated Show resolved Hide resolved
@arcresu
Copy link
Member

arcresu commented May 27, 2019

So should I wait for my PR until MediaFile is done moving?

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.

beets/mediafile.py Outdated Show resolved Hide resolved
@dosoe
Copy link
Contributor Author

dosoe commented May 27, 2019

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: The remote server returned an error: (503) Server Unavailable. Service Unavailable)

@dosoe dosoe closed this May 27, 2019
@dosoe
Copy link
Contributor Author

dosoe commented May 27, 2019

whoops closed accidentally...

@dosoe dosoe reopened this May 27, 2019
@dosoe
Copy link
Contributor Author

dosoe commented May 27, 2019

Actually, I noticed that very few fields use the disambiguation, I wonder why?

@arcresu
Copy link
Member

arcresu commented May 28, 2019

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 artist that we currently treat as single-valued fields. We can work to properly solve the handling of multi-valued fields in beets and fix all these fields consistently. In the short term, a single work for each track (e.g. the first one in MB) is still useful right?

@dosoe
Copy link
Contributor Author

dosoe commented May 28, 2019

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.

@dosoe
Copy link
Contributor Author

dosoe commented May 28, 2019

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.

@sampsyo
Copy link
Member

sampsyo commented May 28, 2019

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.

@arcresu
Copy link
Member

arcresu commented May 28, 2019

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.

@dosoe
Copy link
Contributor Author

dosoe commented May 30, 2019

Sounds fine to me! Commit incoming.

@arcresu
Copy link
Member

arcresu commented May 31, 2019

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.

@sampsyo
Copy link
Member

sampsyo commented May 31, 2019

Merged with changelog! Thanks for your continued attention to this, @dosoe!

@sampsyo sampsyo merged commit 5f88ec5 into beetbox:master May 31, 2019
sampsyo added a commit that referenced this pull request May 31, 2019
add work, work-disambig and work_id tags
sampsyo added a commit that referenced this pull request May 31, 2019
@dosoe dosoe deleted the beets_work_id_2 branch May 31, 2019 12:11
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.

None yet

4 participants