Repository: fennekki/cdparacord

Issue:
Albumartist omitted on multi-artist album where artist matches albumartist
When rippng a CD that has tracks with artist tag matching albumartist tag, and the setting to always tag albumartist is set to false, the generation of albumartist tags is determined per track based on whether their values would be the same.

This logic works for single-artist-albums where no albumartist is wanted for whatever reason, but on multi-artist albums it means the tracks with artist == albumartist will not have albumartist tags created. While this is not a problem in all software, I personally use some pathological cases that actually treat the parts of the album with albumartist tagged, and the parts without, as two separate albums with the same name from the same albumartist. This needs to stop.

Discussion:

Patch:
diff --git a/cdparacord/albumdata.py b/cdparacord/albumdata.py
index 37a9cde..ee1bb37 100644
--- a/cdparacord/albumdata.py
+++ b/cdparacord/albumdata.py
@@ -59,11 +59,23 @@ class Albumdata:
         self._ripdir = albumdata['ripdir']
         self._tracks = []
         counter = 0
+
         for trackdata in albumdata['tracks']:
             counter = counter + 1
             trackdata['tracknumber'] = counter
             self._tracks.append(Track(trackdata))
 
+        # Find out if there are multiple artists by seeing if at least
+        # one of the track artists differs from album artist
+        #
+        # This check used to be per-track only later in the rip process
+        # which caused issues.
+        if any(t.artist != self.albumartist for t in self._tracks):
+            self.multiartist = True
+        else:
+            self.multiartist = False
+
+
     @staticmethod
     def _print_albumdata(albumdata):
         """Print albumdata to fit a terminal.
@@ -306,8 +318,6 @@ class Albumdata:
 
     @classmethod
     def _generate_filename(cls, data, track, tracknumber, config):
-        # TODO: We need the filters here
-        # Also, the templates
         safetyfilter = config.get('safetyfilter')
         target_template = string.Template(config.get('target_template'))
         s = {
diff --git a/cdparacord/rip.py b/cdparacord/rip.py
index 2524a61..73f354f 100644
--- a/cdparacord/rip.py
+++ b/cdparacord/rip.py
@@ -66,9 +66,10 @@ class Rip:
             audiofile = mutagen.File(temp_encoded, easy=True)
             audiofile.add_tags()
 
-        if (track.artist != self._albumdata.albumartist
+        # We only tag albumartist on multi-artist albums, or if we're
+        # set to always tag albumartist.
+        if (self._albumdata.multiartist
                 or self._config.get('always_tag_albumartist')):
-            # We only tag albumartist Sometimes
             audiofile['albumartist'] = self._albumdata.albumartist
 
         # This is information we always save and presumably always have


Test patch:
diff --git a/tests/test_albumdata.py b/tests/test_albumdata.py
index b544f26..2953ad0 100644
--- a/tests/test_albumdata.py
+++ b/tests/test_albumdata.py
@@ -249,6 +249,17 @@ def test_albumdata_tracks(albumdata):
     assert a.albumartist == 'Test Artist'
     assert a.title == 'Test album'
     assert a.date == '2018-01'
+    # This albumdata has a single artist
+    assert a.multiartist == False
+
+    # Also test that we can initialise a multiartist data
+    testdata2 = copy.deepcopy(testdata)
+    testdata2['tracks'][0]['artist'] = 'Test Artist 2'
+    a = albumdata.Albumdata(testdata2)
+    # We now have differing artist and albumartist but otherwise
+    # matching data
+    assert a.multiartist == True
+
 
 
 def test_initialise_albumdata(albumdata):
diff --git a/tests/test_rip.py b/tests/test_rip.py
index 3814f49..17b0cc5 100644
--- a/tests/test_rip.py
+++ b/tests/test_rip.py
@@ -258,6 +258,10 @@ def test_tag_track(monkeypatch, get_fake_config):
         def albumartist(self):
             return 'test'
 
+        @property
+        def multiartist(self):
+            return False
+
         @property
         def title(self):
             return 'test'
@@ -294,7 +298,7 @@ def test_tag_track(monkeypatch, get_fake_config):
         def __init__(*a, **b):
             import mutagen
             raise mutagen.MutagenError('something')
-    
+
     monkeypatch.setattr('mutagen.easyid3.EasyID3', FakeFile2)
     monkeypatch.setattr('mutagen.File', FakeFile)