fixes for write conditions in localtrack tagwriter

musify/libraries/local/track/_tags/writer.py

@@ -159,10 +159,17 @@ def write_tag(self, tag_id: str | None, tag_value: Any, dry_run: bool = True) ->
             return False
 
         if tag_value is None:
-            remove = not dry_run and tag_id in self.file and self.file[tag_id]
-            if remove:
+            if not dry_run and tag_id in self.file and self.file[tag_id] is not None:
                 del self.file[tag_id]
-            return remove
+                return True
+            return False
+
+        return self._write_tag(tag_id=tag_id, tag_value=tag_value, dry_run=dry_run)
+
+    @abstractmethod
+    def _write_tag(self, tag_id: str | None, tag_value: Any, dry_run: bool = True) -> bool:
+        """Implementation of tag writer specific to this file type."""
+        raise NotImplementedError
 
     def write_title(self, source: Track, target: Track, replace: bool = False, dry_run: bool = True) -> int | None:
         """
@@ -176,10 +183,10 @@ def write_title(self, source: Track, target: Track, replace: bool = False, dry_r
         :return: The index number of the conditional that was met to warrant updating the file's tags.
             None if none of the conditions were met.
         """
-        conditionals = {
+        conditionals = [
             source.title is None and target.title is not None,
             replace and source.title != target.title
-        }
+        ]
         if any(conditionals) and self._write_title(track=target, dry_run=dry_run):
             return [i for i, c in enumerate(conditionals) if c][0]
 
@@ -204,10 +211,10 @@ def write_artist(self, source: Track, target: Track, replace: bool = False, dry_
         :return: The index number of the conditional that was met to warrant updating the file's tags.
             None if none of the conditions were met.
         """
-        conditionals = {
+        conditionals = [
             source.artist is None and target.artist is not None,
             replace and source.artist != target.artist
-        }
+        ]
         if any(conditionals) and self._write_artist(track=target, dry_run=dry_run):
             return [i for i, c in enumerate(conditionals) if c][0]
 
@@ -232,10 +239,10 @@ def write_album(self, source: Track, target: Track, replace: bool = False, dry_r
         :return: The index number of the conditional that was met to warrant updating the file's tags.
             None if none of the conditions were met.
         """
-        conditionals = {
+        conditionals = [
             source.album is None and target.album is not None,
             replace and source.album != target.album
-        }
+        ]
         if any(conditionals) and self._write_album(track=target, dry_run=dry_run):
             return [i for i, c in enumerate(conditionals) if c][0]
 
@@ -262,10 +269,10 @@ def write_album_artist(
         :return: The index number of the conditional that was met to warrant updating the file's tags.
             None if none of the conditions were met.
         """
-        conditionals = {
+        conditionals = [
             source.album_artist is None and target.album_artist is not None,
             replace and source.album_artist != target.album_artist
-        }
+        ]
         if any(conditionals) and self._write_album_artist(track=target, dry_run=dry_run):
             return [i for i, c in enumerate(conditionals) if c][0]
 
@@ -290,11 +297,11 @@ def write_track(self, source: Track, target: Track, replace: bool = False, dry_r
         :return: The index number of the conditional that was met to warrant updating the file's tags.
             None if none of the conditions were met.
         """
-        conditionals = {
+        conditionals = [
             source.track_number is None and source.track_total is None and
             (target.track_number is not None or target.track_total is not None),
             replace and (source.track_number != target.track_number or source.track_total != target.track_total)
-        }
+        ]
         if any(conditionals) and self._write_track(track=target, dry_run=dry_run):
             return [i for i, c in enumerate(conditionals) if c][0]
 
@@ -331,7 +338,7 @@ def write_genres(self, source: Track, target: Track, replace: bool = False, dry_
         :return: The index number of the conditional that was met to warrant updating the file's tags.
             None if none of the conditions were met.
         """
-        conditionals = {source.genres is None and bool(target.genres), replace and source.genres != target.genres}
+        conditionals = [source.genres is None and bool(target.genres), replace and source.genres != target.genres]
         if any(conditionals) and self._write_genres(track=target, dry_run=dry_run):
             return [i for i, c in enumerate(conditionals) if c][0]
 
@@ -360,10 +367,10 @@ def write_date(
         """
         # date is just a composite of year + month + day, can safely ignore this in the conditionals
         values_exist = any({target.year is not None, target.month is not None, target.day is not None})
-        conditionals = {
+        conditionals = [
             source.year is None and source.month is None and source.day is None and values_exist,
             replace and (source.year != target.year or source.month != target.month or source.day != target.day)
-        }
+        ]
 
         if not any(conditionals):
             return
@@ -419,11 +426,11 @@ def write_bpm(self, source: Track, target: Track, replace: bool = False, dry_run
         source_bpm = int(source.bpm if source.bpm is not None else 0)
         target_bpm = int(target.bpm if target.bpm is not None else 0)
 
-        conditionals = {
+        conditionals = [
             source.bpm is None and target.bpm is not None and target_bpm > 30,
             source_bpm < 30 < target_bpm,
             replace and source_bpm != target_bpm
-        }
+        ]
         if any(conditionals) and self._write_bpm(track=target, dry_run=dry_run):
             return [i for i, c in enumerate(conditionals) if c][0]
 
@@ -448,7 +455,7 @@ def write_key(self, source: Track, target: Track, replace: bool = False, dry_run
         :return: The index number of the conditional that was met to warrant updating the file's tags.
             None if none of the conditions were met.
         """
-        conditionals = {source.key is None and target.key is not None, replace and source.key != target.key}
+        conditionals = [source.key is None and target.key is not None, replace and source.key != target.key]
 
         if any(conditionals) and self._write_key(track=target, dry_run=dry_run):
             return [i for i, c in enumerate(conditionals) if c][0]
@@ -474,11 +481,11 @@ def write_disc(self, source: Track, target: Track, replace: bool = False, dry_ru
         :return: The index number of the conditional that was met to warrant updating the file's tags.
             None if none of the conditions were met.
         """
-        conditionals = {
+        conditionals = [
             source.disc_number is None and source.disc_total is None and
             (target.disc_number is not None or target.disc_total is not None),
             replace and (source.disc_number != target.disc_number or source.disc_total != target.disc_total)
-        }
+        ]
         if any(conditionals) and self._write_disc(track=target, dry_run=dry_run):
             return [i for i, c in enumerate(conditionals) if c][0]
 
@@ -518,10 +525,10 @@ def write_compilation(
         :return: The index number of the conditional that was met to warrant updating the file's tags.
             None if none of the conditions were met.
         """
-        conditionals = {
+        conditionals = [
             source.compilation is None and target.compilation is not None,
             replace and source.compilation != target.compilation
-        }
+        ]
         if any(conditionals) and self._write_compilation(track=target, dry_run=dry_run):
             return [i for i, c in enumerate(conditionals) if c][0]
 
@@ -546,10 +553,10 @@ def write_comments(self, source: Track, target: Track, replace: bool = False, dr
         :return: The index number of the conditional that was met to warrant updating the file's tags.
             None if none of the conditions were met.
         """
-        conditionals = {
+        conditionals = [
             source.comments is None and bool(target.comments),
             replace and source.comments != target.comments
-        }
+        ]
         if any(conditionals) and self._write_comments(track=target, dry_run=dry_run):
             return [i for i, c in enumerate(conditionals) if c][0]
 
@@ -578,7 +585,7 @@ def write_uri(self, source: Track, target: Track, replace: bool = False, dry_run
         if not self.remote_wrangler:
             return
 
-        conditionals = {source.uri != target.uri or source.has_uri != target.has_uri}
+        conditionals = [source.uri != target.uri or source.has_uri != target.has_uri]
 
         if any(conditionals) and self._write_uri(track=target, dry_run=dry_run):
             return [i for i, c in enumerate(conditionals) if c][0]
@@ -606,7 +613,7 @@ def write_images(self, source: Track, target: Track, replace: bool = False, dry_
         :return: The index number of the conditional that was met to warrant updating the file's tags.
             None if none of the conditions were met.
         """
-        conditionals = {source.has_image is False and bool(target.image_links), replace and bool(target.image_links)}
+        conditionals = [source.has_image is False and bool(target.image_links), replace and bool(target.image_links)]
 
         if any(conditionals) and self._write_images(track=target, dry_run=dry_run):
             return [i for i, c in enumerate(conditionals) if c][0]

musify/libraries/local/track/flac.py

@@ -40,11 +40,7 @@ class _FLACTagWriter(TagWriter[mutagen.flac.FLAC]):
 
     __slots__ = ()
 
-    def write_tag(self, tag_id: str | None, tag_value: Any, dry_run: bool = True) -> bool:
-        result = super().write_tag(tag_id=tag_id, tag_value=tag_value, dry_run=dry_run)
-        if result is not None:
-            return result
-
+    def _write_tag(self, tag_id: str | None, tag_value: Any, dry_run: bool = True) -> bool:
         if not dry_run:
             if isinstance(tag_value, (list, set, tuple)):
                 self.file[tag_id] = list(map(str, tag_value))

musify/libraries/local/track/m4a.py

@@ -75,11 +75,7 @@ class _M4ATagWriter(TagWriter[mutagen.mp4.MP4]):
 
     __slots__ = ()
 
-    def write_tag(self, tag_id: str | None, tag_value: Any, dry_run: bool = True) -> bool:
-        result = super().write_tag(tag_id=tag_id, tag_value=tag_value, dry_run=dry_run)
-        if result is not None:
-            return result
-
+    def _write_tag(self, tag_id: str | None, tag_value: Any, dry_run: bool = True) -> bool:
         if not dry_run:
             if tag_id.startswith("----:com.apple.iTunes"):
                 self.file[tag_id] = [
@@ -113,7 +109,8 @@ def _write_date(self, track: LocalTrack, dry_run: bool = True) -> tuple[bool, bo
         return date, year, month, day
 
     def _write_bpm(self, track: LocalTrack, dry_run: bool = True) -> bool:
-        return self.write_tag(next(iter(self.tag_map.bpm), None), int(track.bpm), dry_run)
+        bpm = int(track.bpm) if track.bpm is not None else None
+        return self.write_tag(next(iter(self.tag_map.bpm), None), bpm, dry_run)
 
     def _write_disc(self, track: LocalTrack, dry_run: bool = True) -> bool:
         tag_id = next(iter(self.tag_map.disc_number), None)

musify/libraries/local/track/mp3.py

@@ -83,11 +83,7 @@ def _clear_tag(self, tag_name: str, dry_run: bool = True) -> bool:
 
         return removed
 
-    def write_tag(self, tag_id: str | None, tag_value: Any, dry_run: bool = True) -> bool:
-        result = super().write_tag(tag_id=tag_id, tag_value=tag_value, dry_run=dry_run)
-        if result is not None:
-            return result
-
+    def _write_tag(self, tag_id: str | None, tag_value: Any, dry_run: bool = True) -> bool:
         if not dry_run:
             self.file[tag_id] = getattr(mutagen.id3, tag_id)(3, text=str(tag_value))
         return True

musify/libraries/local/track/wma.py

@@ -85,11 +85,7 @@ class _WMATagWriter(TagWriter[mutagen.asf.ASF]):
 
     __slots__ = ()
 
-    def write_tag(self, tag_id: str | None, tag_value: Any, dry_run: bool = True) -> bool:
-        result = super().write_tag(tag_id=tag_id, tag_value=tag_value, dry_run=dry_run)
-        if result is not None:
-            return result
-
+    def _write_tag(self, tag_id: str | None, tag_value: Any, dry_run: bool = True) -> bool:
         if not dry_run:
             if isinstance(tag_value, (list, set, tuple)):
                 if all(isinstance(v, mutagen.asf.ASFByteArrayAttribute) for v in tag_value):

tests/libraries/local/library/testers.py

@@ -19,11 +19,6 @@ async def test_save_playlists(library: LocalLibrary):
 
         results = await library.save_playlists(dry_run=True)
 
-        for pl, result in results.items():
-            print(pl.name, result)
-        for pl in playlists:
-            print(pl.name, len(pl))
-
         assert len(results) == len(library.playlists)
         for pl, result in results.items():
             if pl not in playlists:

tests/libraries/local/track/test_track.py

@@ -1,6 +1,7 @@
 from copy import copy
 from datetime import datetime, date
 from pathlib import Path
+from random import choice
 
 import mutagen
 import pytest
@@ -337,10 +338,18 @@ def assert_track_tags_equal(actual: LocalTrack, expected: LocalTrack):
         assert actual.track_number == expected.track_number, "track_number"
         assert actual.track_total == expected.track_total, "track_total"
         assert actual.genres == expected.genres, "genres"
+
         assert actual.date == expected.date, "date"
         assert actual.year == expected.year, "year"
-        assert actual.month == expected.month, "month"
-        assert actual.day == expected.day, "day"
+        if actual.year is not None:
+            assert actual.month == expected.month, "month"
+        else:
+            assert actual.month is None
+        if actual.year is not None and actual.month is not None:
+            assert actual.day == expected.day, "day"
+        else:
+            assert actual.day is None
+
         assert actual.bpm == expected.bpm, "bpm"
         assert actual.key == expected.key, "key"
         assert actual.disc_number == expected.disc_number, "disc_number"
@@ -503,8 +512,12 @@ async def test_update_tags_results(self, track: LocalTrack):
         track.album = "new album artist"
         track.track_number += 2
         track.genres = ["Big Band", "Swing"]
-        track.year += 10
-        track.bpm += 10
+        if choice([True, False]):
+            track.year = None
+            track.bpm += 10
+        else:
+            track.year += 10
+            track.bpm = None
         track.key = "F#"
         track.disc_number += 5
 
@@ -523,7 +536,7 @@ async def test_update_tags_results(self, track: LocalTrack):
         }
         assert set(result.updated) == expected_tags
 
-        self.assert_track_tags_equal(track, copy(track_original))
+        self.assert_track_tags_equal(copy(track_original), track)
 
         track.artist = "new artist"
         track.album_artist = "new various"