Repository: beetbox/mediafile

Issue:
mb_artistid and mb_albumartistid should be lists
Hey!
When trying to fix my PR (#42) I found error I can't fix myself.

`mb_artistid` and `mb_albumartistid` should be lists!
`mediafile`'s version:
```python
>>> mf = MediaFile('/home/juniorjpdj/temp/Szpadyzor crew.flac')
>>> mf.mb_artistid
'164ed128-525d-4798-883f-be13502de86b'
>>> mf.mb_albumartistid
'164ed128-525d-4798-883f-be13502de86b'
```

Picard is tagging it like that (mutagen dump):
FLAC:  `'musicbrainz_artistid': ['164ed128-525d-4798-883f-be13502de86b', 'ded6b91b-26d8-4bc0-8aa9-fab9b54cd157'],`
MP3 (ID3v2.3): ` 'TXXX:MusicBrainz Artist Id': TXXX(encoding=<Encoding.UTF16: 1>, desc='MusicBrainz Artist Id', text=['ca780457-5bf6-4a7d-95fa-0d0d4920f26f/258a67d1-172d-4d16-8d6c-09efde60896f/af262d86-542d-4864-8527-083065166d6c']),`

Fixing FLAC parsing is easy I can't make it work in MP3 case.

Current code is returning only the first author in case of FLAC and `'/'` separated string in case of MP3.

Helping fixing this will also help me solve some problems with #42 ;D

Discussion:
JuniorJPDJ: Rebased on top of master.
sampsyo: Alright, so this is the tricky one. On the surface, it's a simple change; unfortunately, the interaction with reality is complicated.

Probably most importantly (and as discussed elsewhere), changing the type of these existing fields would break current software that depends on these fields—most importantly, it would break beets (and beets plugins). This change would be tricky to get right everywhere, especially considering third-party plugins that might rely on the string behavior. Providing a second interface to get list-style access to the same data would be awesome.

Second, there is a broader issue here about how to deal with multi-value tags in MediaFile. It would be really great to come up with some sort of consistent way to expose list-formatted data. However, there are many complicating factors:

* *Most* fields can be made into lists for *most* formats, but not all. So a blanket policy of making things lists would end up with weird exceptions.
* Some fields don't feel like they would meaningfully *want* to be lists, but exactly which is a frustratingly human judgment call. (Should multiple titles be supported? Multiple release dates?)
* This sounds silly, but non-list values are just plain easier to deal with than list values. The common case for genre, for instance, is that it works just fine to expose a single string—and MediaFile makes it a lot easier to deal with when all you want is that common case. More advanced use cases definitely would like to store multiple genres. But a good design would "make the easy things easy and the hard things possible."

So generally speaking, there is a deeper problem to be solved here about how to expose _both_ list-shaped and non-listy versions of data—for different use cases and different situations. I honestly don't know what to do about it. But I confess some discomfort with introducing lists of data without more clarity on what the grand plan is.
JuniorJPDJ: Actual problem with those values is that files are already tagged with this fields as multi-valued by most of software and I'm not sure why those are NOT lists at the first place. Musicbrainz itself specifies those tags as multi-valued as you need to write information about all artist IDs to the same tag. `mediafile` seems to expose only one of all values in lists (first one AFAIR) in case of most of formats and some concatenated strings in case of ID3v2.3 (as shown in #43) - this behaviour is bad and inconsistent.

> Most fields can be made into lists for most formats, but not all. So a blanket policy of making things lists would end up with weird exceptions.

Is there any format beyond `ID3v<=2.3` that doesn't support those? This was the only format I had problems when testing this tbh and this field has already applied solution for problem with `ID3v<=2.3`.

> Some fields don't feel like they would meaningfully want to be lists, but exactly which is a frustratingly human judgment call. (Should multiple titles be supported? Multiple release dates?)

I don't get what you are talking about - those fields are not lists and are not supposed to be lists. Only few of fileds are made to be lists and part of those were already implemented before I tried to deal with multi-valued fields ;O

In sake of backwards compatibility I probably could rename those fields to `mb_artistids` and `mb_albumartistids` and provide backward-compatible versions of those in similar fashion like `genre` and `genres` are created, but IMO it still would be bad (better than at this point tho).

I'm not sure what will happen if you for example overwrite first value using single-value tag when list would contain more than one value. Will it remove other values? Leave other values untouched and just change first? That creates loads of bugs in end software.

> But I confess some discomfort with introducing lists of data without more clarity on what the grand plan is.

There's no grand plan ;D
I just wanted to fix bug I found as I gonna use `mediafile` in my software.

And yes, this is a bug - `mediafile` was interpreting files tagged by other software (in case of my tests - Picard) in inconsistent and not intended way, probably also destroying already tagged files (I haven't checked it tho).
JuniorJPDJ: I just renamed lists to plural forms and added `.single_field()` fields on singular fields.
It looks like even compatibility with behaviour shown above is kept!

```
>>> mf.mb_albumartistids
['164ed128-525d-4798-883f-be13502de86b', 'ded6b91b-26d8-4bc0-8aa9-fab9b54cd157']
>>> mf.mb_albumartistid
'164ed128-525d-4798-883f-be13502de86b'
>>> mf.mb_albumartistid = '164ed128-525d-4798-883f-be13502de86b'
>>> mf.mb_albumartistid
'164ed128-525d-4798-883f-be13502de86b'
>>> mf.mb_albumartistids
['164ed128-525d-4798-883f-be13502de86b']
```

I also rolled back tests as my changes are not needed anymore as main tags work as before.

What do you think about it now?

Patch:
diff --git a/mediafile.py b/mediafile.py
index a9e5e8e..90e54a8 100644
--- a/mediafile.py
+++ b/mediafile.py
@@ -1994,18 +1994,27 @@ class MediaFile(object):
         StorageStyle('MUSICBRAINZ_ALBUMID'),
         ASFStorageStyle('MusicBrainz/Album Id'),
     )
-    mb_artistid = MediaField(
-        MP3DescStorageStyle(u'MusicBrainz Artist Id'),
-        MP4StorageStyle('----:com.apple.iTunes:MusicBrainz Artist Id'),
-        StorageStyle('MUSICBRAINZ_ARTISTID'),
+    mb_artistids = ListMediaField(
+        MP3ListDescStorageStyle(u'MusicBrainz Artist Id', split_v23=True),
+        MP4ListStorageStyle('----:com.apple.iTunes:MusicBrainz Artist Id'),
+        ListStorageStyle('MUSICBRAINZ_ARTISTID'),
         ASFStorageStyle('MusicBrainz/Artist Id'),
     )
-    mb_albumartistid = MediaField(
-        MP3DescStorageStyle(u'MusicBrainz Album Artist Id'),
-        MP4StorageStyle('----:com.apple.iTunes:MusicBrainz Album Artist Id'),
-        StorageStyle('MUSICBRAINZ_ALBUMARTISTID'),
+    mb_artistid = mb_artistids.single_field()
+
+    mb_albumartistids = ListMediaField(
+        MP3ListDescStorageStyle(
+            u'MusicBrainz Album Artist Id',
+            split_v23=True,
+        ),
+        MP4ListStorageStyle(
+            '----:com.apple.iTunes:MusicBrainz Album Artist Id',
+        ),
+        ListStorageStyle('MUSICBRAINZ_ALBUMARTISTID'),
         ASFStorageStyle('MusicBrainz/Album Artist Id'),
     )
+    mb_albumartistid = mb_albumartistids.single_field()
+
     mb_releasegroupid = MediaField(
         MP3DescStorageStyle(u'MusicBrainz Release Group Id'),
         MP4StorageStyle('----:com.apple.iTunes:MusicBrainz Release Group Id'),


Test patch:
diff --git a/test/test_mediafile.py b/test/test_mediafile.py
index 269cdd2..9e1fdd7 100644
--- a/test/test_mediafile.py
+++ b/test/test_mediafile.py
@@ -991,8 +991,10 @@ class MediaFieldTest(unittest.TestCase):
 
     def test_known_fields(self):
         fields = list(ReadWriteTestBase.tag_fields)
-        fields.extend(('encoder', 'images', 'genres', 'albumtype', 'artists',
-                       'albumartists', 'url'))
+        fields.extend(
+            ('encoder', 'images', 'genres', 'albumtype', 'artists',
+             'albumartists', 'url', 'mb_artistids', 'mb_albumartistids')
+        )
         assertCountEqual(self, MediaFile.fields(), fields)
 
     def test_fields_in_readable_fields(self):