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

multi-valued tags for artists + albumartists sort and credits #69

Merged
merged 5 commits into from
Jun 28, 2023

Conversation

jmbannon
Copy link
Contributor

@jmbannon jmbannon commented Jun 18, 2023

As title, for the PR: beetbox/beets#4743

On Ubuntu, I'm hitting a pre-existing test failure on WAVETest.test_read_audio_properties when reading the bitrate tag: 88200 != 705600

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.

Hi there! Thanks for getting this started!

I notice that you have used "plural" names for the underlying on-disk metadata formats for each field here. That means that, for example, artist_sort will use completely different metadata from artists_sort: one will never affect the other. Is this intentional? Is there external software you have in mind that is expecting these fields to work this way?

It isn't how we do it for other fields that have both "singular" and "plural" equivalents. For example, here is the mapping for the genre singular field, which is based on genres (plural):

mediafile/mediafile.py

Lines 1811 to 1817 in 57a330e

genres = ListMediaField(
MP3ListStorageStyle('TCON'),
MP4ListStorageStyle('\xa9gen'),
ListStorageStyle('GENRE'),
ASFStorageStyle('WM/Genre'),
)
genre = genres.single_field()

It uses the same underlying storage as genres, meaning that we stay interoperable with other software using those fields. Would that be appropriate here?

I realize we don't do this for artists, but perhaps we should…

mediafile.py Outdated Show resolved Hide resolved
mediafile.py Outdated Show resolved Hide resolved
@sampsyo
Copy link
Member

sampsyo commented Jun 28, 2023

Looks good overall! I wouldn't worry too much about the bitrate mismatch; it likely has to do with the underlying version of Mutagen (but in any case it's not a big deal).

Could you please add a quick changelog entry here?
https://github.com/beetbox/mediafile/blob/master/docs/index.rst#changelog

Then we should be good to go!

@jmbannon
Copy link
Contributor Author

@sampsyo Done!

@sampsyo
Copy link
Member

sampsyo commented Jun 28, 2023

Excellent; thanks!

@sampsyo sampsyo merged commit 8a1b6af into beetbox:master Jun 28, 2023
@jmbannon
Copy link
Contributor Author

When you get some time @sampsyo , mind updating pypi with 0.11.1? Then I can update the beets PR's pip dep with this change

@sampsyo
Copy link
Member

sampsyo commented Jul 3, 2023

I've actually published it as v0.12.0 (following the SemVer recommendation to bump the minor version for new features). Thanks again!

@JOJ0 JOJ0 changed the title multi-tags for artists + albumartists sort and credits multi-valued tags for artists + albumartists sort and credits Sep 9, 2023
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.

2 participants