Store secondary release types - #4045
Conversation
sampsyo
left a comment
sampsyo
left a comment
There was a problem hiding this comment.
Cool; thank you for getting this ball rolling! This seems like a promising direction.
I like the idea of adding a new albumtypes field. We don't currently have a lot of machinery for supporting list-valued fields, so a comma-separated string is likely the right way to go for now. We can consider more ambitious ways to store or query the data in the future… a simple hack could involve a comma-aware query type.
For now, I would prefer not to add the more sophisticated atype field for formatting. We can explore designs like this in subsequent PRs, but I would love to merge the less-tricky part, just about fetching and storing the underlying data, before worrying about that. In particular, while fetching/storing the data is universal and belongs in "core" beets, the range of configuration and display styles are much more personal and might be best handled by a plugin…
| self.tracks = tracks | ||
| self.asin = asin | ||
| self.albumtype = albumtype | ||
| self.albumtypes = albumtypes |
There was a problem hiding this comment.
There's actually no need to add albumtypes as an explicit parameter here; it can go into **kwargs. We have a bunch of fields listed here purely for legacy reasons; the AlbumInfo class is now "flexible" instead of baking in a fixed set of fields.
There was a problem hiding this comment.
Gotcha! I've removed what I had added here, seems to work still!
| log.debug('secondary MB release type(s): ' + ', '.join( | ||
| [secondarytype.lower() for secondarytype in | ||
| release['release-group']['secondary-type-list']])) | ||
| info.albumtypes += ',' + ','.join( |
There was a problem hiding this comment.
Using += ',' seems to run the risk of starting the string with a comma, if the album is missing a primary-type?
Maybe a simpler alternative would be to build up a list, albumtypes, that starts out empty and is then expanded under both if conditions, and then join it all together at the end.
There was a problem hiding this comment.
Fair point! I had thought of that, but forgot about it just as quickly.
Gotcha, I've removed
Agreed. I've extracted |
edgars-supe
changed the title
| 'mb_releasetrackid': types.STRING, | ||
| 'trackdisambig': types.STRING, | ||
| 'albumtype': types.STRING, | ||
| 'albumtypes': types.STRING, |
There was a problem hiding this comment.
I had to add this, because otherwise beet import -A would fail, because it sets values on an Item, iterating over Album.item_keys.
sampsyo
left a comment
sampsyo
left a comment
There was a problem hiding this comment.
Awesome; looking great! I've added one small suggestion. Could you also please add a changelog entry to changelog.rst?
In answer to your question about the plugin, it's completely OK to either maintain a plugin separately or open a PR to propose inclusion in beets. For this one, it's simple enough that it would be pretty easy to include with beets if you're interested! It does involve a teensy bit of extra work, such as writing a documentation page that matches the existing plugin docs, but that shouldn't be too hard.
| 'mb_releasetrackid': types.STRING, | ||
| 'trackdisambig': types.STRING, | ||
| 'albumtype': types.STRING, | ||
| 'albumtypes': types.STRING, |
| release['release-group']['secondary-type-list']])) | ||
| for sec_type in release['release-group']['secondary-type-list']: | ||
| albumtypes.append(sec_type.lower()) | ||
| info.albumtypes = ','.join(albumtypes) |
There was a problem hiding this comment.
It occurs to me somewhat belatedly, after reviewing #4044, that we've been using an existing convention for storing list-like data in string fields: namely, separating values with ; (semicolon-space) as opposed to just , (comma). I think this makes for somewhat more legible strings for humans and also is slightly less likely to get confused (because , seems more likely to occur within an individual element). Can we do that here?
There was a problem hiding this comment.
No problem; done!
|
I've added a line to Okay, gotcha! I'll add it as a plugin to beets in a separate PR then. I think many people might find such a plugin quite useful out of the box; better than bothering with installing external plugins. |
|
Sounds great! Working on that in a separate PR will also be a good way to get other users' feedback about how the configuration should work, etc. |
2 participants
Description
Closes #2200.
I've implemented some rudimentary processing for primary and secondary types,
as well as a configurable field for use in paths. I haven't written any docs or tests yet, because I'm not sure if it should be done this way (it's my first foray into beets's source code and Python).Solution
I've added
albumtypesto theAlbumschema andAlbumInfo. Instead of simply logging the primary and secondary release types, I make a comma-separated string of them and store it in the new field.Then, I added a custom field (?) to(removed, see comment)Album,atypes, that outputs the album types according to the config.Notes and questions
beet ls albumtypes:remixreturns "On Remixes", as doesalbumtypes:ep,remix, butremix,epdoesn't.albumtypes) as a list (or set) instead of a plainfoo,barstring?To Do
docs/to describe it.)docs/changelog.rstnear the top of the document.)