ytdl-sub icon indicating copy to clipboard operation
ytdl-sub copied to clipboard

Add list support for music_tags plugin

Open jmbannon opened this issue 2 years ago • 5 comments

id3v2.4 tags allow lists. We should be able to write those.

The music_tags plugin uses beets' mediafile package: https://github.com/beetbox/mediafile/blob/master/mediafile.py You can see some fields are ListMediaField. Those are the ones that should be able to support list values in ytdl-sub

Task: Support id3v2.4 lists by specifying a list in music_tags.tags.<some_field> https://github.com/jmbannon/ytdl-sub/blob/master/src/ytdl_sub/plugins/music_tags.py#L76

subtask: These fields should support both strings and list of strings. https://github.com/jmbannon/ytdl-sub/blob/master/src/ytdl_sub/plugins/music_tags.py#L42

subtask: We should verify the specified field supports lists prior to any downloading. This would need to happen in the

jmbannon avatar Jul 22 '22 07:07 jmbannon

Can I take this issue?

MariaMozgunova avatar Jul 25 '22 18:07 MariaMozgunova

Can I take this issue?

Lets get your other PR in first :-)

jmbannon avatar Jul 25 '22 22:07 jmbannon

Do you still want to take this @MariaMozgunova ?

jmbannon avatar Jul 27 '22 17:07 jmbannon

@jmbannon Could you answer a few questions?

There is an example https://github.com/jmbannon/ytdl-sub/blob/master/src/ytdl_sub/plugins/music_tags.py#L30-L33 for non-list tags. What will it look like for the list tags?

For the first subtask, the new FormatterValidator needs to be declared?

The second subtask seems to be incomplete.

MariaMozgunova avatar Jul 29 '22 09:07 MariaMozgunova

@jmbannon Could you answer a few questions?

There is an example https://github.com/jmbannon/ytdl-sub/blob/master/src/ytdl_sub/plugins/music_tags.py#L30-L33 for non-list tags. What will it look like for the list tags?

It should be either a single value per tag like it is now OR a list of values. In the backend, treat both cases as a list of StringFormatValidators.

I added this change so yaml input can support single value (casts to list of 1) or lists for any ListValidator: https://github.com/jmbannon/ytdl-sub/pull/126

For the first subtask, the new FormatterValidator needs to be declared?

I think two validators should be made: a StringFormatListValidator, and a MusicTagValidator that inherits from it. Each music tag will technically be a list, but treat it as a value if the list length is only one.

The second subtask seems to be incomplete.

My bad. Was going to say the MusicTagValidator should ensure the tag supports lists (i.e. artists does, not artist). See the MediaFile dep source code to see which ones are lists or not. This validation should be in the MusicTagValidator init.

Hopefully that makes sense 😅

jmbannon avatar Jul 30 '22 06:07 jmbannon