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

An opinionated fix for "the albumtype(s) problem" #5075

Draft
wants to merge 7 commits into
base: master
Choose a base branch
from

Conversation

JOJ0
Copy link
Member

@JOJ0 JOJ0 commented Jan 10, 2024

Description

Addresses parts of #4715

This fix is of the type "works for me" and I'd like to open the discussion and test for others. In my opinion most of the problems we have with albumtypes and albumtype fighting against each other is because it just does not make sense to save both of them in beets. No matter where the real problem is, in the end it should be sufficient to save the data once in the beets library, hence only in the multifield albumtypes. I'm proposing to kick out albumtype from beets entirely!

This fix does not require any changes to mediafile

Furthermore this fixes saving albumtypes for the Discogs, the Spotify and the Deezer plugin. Previously populating albumtype was implemented theoratically but broken, and albumtypes was not implemented. (I only recently realized this while working on this fix, please tell me if there is open issue for that already, so I can attach them to this PR, thanks!)

A nice side effect of the latter fix is that files that where tagged using one of those plugins, can finally be used correctly by the albumtypes plugin as well 🎉

To Do

  • Documentation.
  • Changelog
  • Fix ambiguation field to use albumtypes instead of albumtype
  • Fix existing Tests accordingly.

We fetch albumtypes from another endpoint anyway.
instead of (single) albumtype string field.
instead of (single) albumtype string attribute.

According to Spotify's API docs the album endpoint will always return a
single value as a string.
instead of (single) albumtype string attribute.

According to Deezer's API docs the album endpoint will always return a
single value as a string.
Remove from AlbumInfo and TrackInfo classes.
@arsaboo
Copy link
Contributor

arsaboo commented Mar 14, 2024

This albumtype issue is really annoying, and I am perfectly fine with this approach. One suggestion - to avoid breaking changes to other plugins, can we set albumtypes = albumtype if the plugin only updates albumtype.

@randoragon
Copy link

Not a contributor, nor is my knowledge of beets internals on a sufficient level to offer technical insight, but as someone who's been using this project for several years (thanks!), this fix seems fine. I don't care for the albumtype field.

@JOJ0
Copy link
Member Author

JOJ0 commented Mar 18, 2024

This albumtype issue is really annoying, and I am perfectly fine with this approach. One suggestion - to avoid breaking changes to other plugins, can we set albumtypes = albumtype if the plugin only updates albumtype.

I'm not sure if it's that easy. Will assigning the string from albumtype to the "multi-field" albumtypes work? You could investigate that in the initial PR that made albumtypes a proper multi-field: #4582

@JOJ0
Copy link
Member Author

JOJ0 commented Mar 18, 2024

Not a contributor, nor is my knowledge of beets internals on a sufficient level to offer technical insight, but as someone who's been using this project for several years (thanks!), this fix seems fine. I don't care for the albumtype field.

Initially I thought I don't care about it either, but now I learned that we can't simply get rid of it, there is too many plugins that still want to use it! So, yeah basically what @arsaboo said is a good idea, let's discuss the technical ways to do that.

@arsaboo
Copy link
Contributor

arsaboo commented Mar 18, 2024

Just thinking aloud here.

How about having some helper function to translate between albumtype and albumtypes? If any plugin sets albumtype, we use that information to set the albumtypes and vice versa. In the db, we only store albumtypes.

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.

3 participants