Merge pull request #2029 from rdswift/allow_empty_strings_in_multi-value

PICARD-2392: Allow empty strings in multi-value variables

picard/metadata.py

@@ -458,8 +458,9 @@ class Metadata(MutableMapping):
         name = self.normalize_tag(name)
         if isinstance(values, str) or not isinstance(values, Iterable):
             values = [values]
-        values = [str(value) for value in values if value or value == 0]
-        if values:
+        values = [str(value) for value in values if value or value == 0 or value == '']
+        # Remove if there is only a single empty or blank element.
+        if values and (len(values) > 1 or values[0]):
             self._store[name] = values
             self.deleted_tags.discard(name)
         elif name in self._store:

picard/script/functions.py

@@ -1496,3 +1496,24 @@ _Since Picard 2.7_"""
 def func_is_multi(parser, multi):
     multi_value = MultiValue(parser, multi, MULTI_VALUED_JOINER)
     return '' if len(multi_value) < 2 else '1'
+
+
+@script_function(eval_args=True, documentation=N_(
+    """`$cleanmulti(name)`
+
+Removes all empty string elements from the multi-value variable.
+
+Example:
+
+    $setmulti(test,one; ; two; three)
+    $cleanmulti(test)
+
+Result: Sets the value of 'test' to ["one", "two", "three"].
+
+_Since Picard 2.8_"""
+))
+def func_cleanmulti(parser, multi):
+    name = normalize_tagname(multi)
+    values = [str(value) for value in parser.context.getall(name) if value or value == 0]
+    parser.context[multi] = values
+    return ""

test/test_script.py

@@ -427,16 +427,16 @@ class ScriptParserTest(PicardTestCase):
         self.assertEqual("", self.parser.eval("$setmulti(test,%source%)", context))  # no return value
         self.assertEqual(context.getall("source"), context.getall("test"))
 
-    def test_cmd_setmulti_will_remove_empty_items(self):
+    def test_cmd_setmulti_will_keep_empty_items(self):
         context = Metadata()
         context["source"] = ["", "multi", ""]
         self.assertEqual("", self.parser.eval("$setmulti(test,%source%)", context))  # no return value
-        self.assertEqual(["multi"], context.getall("test"))
+        self.assertEqual(["", "multi", ""], context.getall("test"))
 
     def test_cmd_setmulti_custom_splitter_string(self):
         context = Metadata()
         self.assertEqual("", self.parser.eval("$setmulti(test,multi##valued##test##,##)", context))  # no return value
-        self.assertEqual(["multi", "valued", "test"], context.getall("test"))
+        self.assertEqual(["multi", "valued", "test", ""], context.getall("test"))
 
     def test_cmd_setmulti_empty_splitter_does_nothing(self):
         context = Metadata()
@@ -558,6 +558,9 @@ class ScriptParserTest(PicardTestCase):
         context["genre"] = ["Electronic", "Jungle", "Bardcore"]
         self.assertScriptResultEquals("$replacemulti(%genre%,Bardcore,Hardcore)", "Electronic; Jungle; Hardcore", context)
 
+        context["test"] = ["One", "Two", "Three"]
+        self.assertScriptResultEquals("$replacemulti(%test%,Two,)", "One; Three", context)
+
         context["test"] = ["One", "Two", "Three"]
         self.assertScriptResultEquals("$replacemulti(%test%,Four,Five)", "One; Two; Three", context)
 
@@ -947,6 +950,9 @@ class ScriptParserTest(PicardTestCase):
         context["baz"] = []
         self.assertScriptResultEquals("$lenmulti(%baz%)", "0", context)
         self.assertScriptResultEquals("$lenmulti(%baz%,:)", "0", context)
+        # Test empty multi-value elements
+        context["baz"] = ["one", "", "three"]
+        self.assertScriptResultEquals("$lenmulti(%baz%)", "3", context)
         # Test missing name
         self.assertScriptResultEquals("$lenmulti(,)", "0", context)
         self.assertScriptResultEquals("$lenmulti(,:)", "0", context)
@@ -1808,3 +1814,56 @@ class ScriptParserTest(PicardTestCase):
             self.parser.eval("$is_multi()")
         with self.assertRaisesRegex(ScriptError, areg):
             self.parser.eval("$is_multi(a,)")
+
+    def test_cmd_cleanmulti(self):
+        context = Metadata()
+        context["foo"] = ["", "one", "two"]
+        context["bar"] = ["one", "", "two"]
+        context["baz"] = ["one", "two", ""]
+
+        # Confirm initial values
+        self.assertScriptResultEquals("%foo%", "; one; two", context)
+        self.assertScriptResultEquals("%bar%", "one; ; two", context)
+        self.assertScriptResultEquals("%baz%", "one; two; ", context)
+        # Test cleaned values
+        self.assertScriptResultEquals("$cleanmulti(foo)%foo%", "one; two", context)
+        self.assertScriptResultEquals("$cleanmulti(bar)%bar%", "one; two", context)
+        self.assertScriptResultEquals("$cleanmulti(baz)%baz%", "one; two", context)
+
+        # Text clean with only empty string elements
+        context["foo"] = ["", "", ""]
+
+        # Confirm initial values
+        self.assertScriptResultEquals("%foo%", "; ; ", context)
+        # Test cleaned values
+        self.assertScriptResultEquals("$cleanmulti(foo)%foo%", "", context)
+
+        # Test clean with indirect argument
+        context["foo"] = ["", "one", "two"]
+        context["bar"] = "foo"
+
+        # Confirm initial values
+        self.assertScriptResultEquals("%foo%", "; one; two", context)
+        # Test cleaned values
+        self.assertScriptResultEquals("$cleanmulti(%bar%)%foo%", "one; two", context)
+
+        # Test clean with non-multi argument
+        context["foo"] = "one"
+        context["bar"] = "one; ; two"
+        context["baz"] = ""
+
+        # Confirm initial values
+        self.assertScriptResultEquals("%foo%", "one", context)
+        self.assertScriptResultEquals("%bar%", "one; ; two", context)
+        self.assertScriptResultEquals("%baz%", "", context)
+        # Test cleaned values
+        self.assertScriptResultEquals("$cleanmulti(foo)%foo%", "one", context)
+        self.assertScriptResultEquals("$cleanmulti(bar)%bar%", "one; ; two", context)
+        self.assertScriptResultEquals("$cleanmulti(baz)%baz%", "", context)
+
+        # Tests with invalid number of arguments
+        areg = r"^\d+:\d+:\$cleanmulti: Wrong number of arguments for \$cleanmulti: Expected exactly 1, "
+        with self.assertRaisesRegex(ScriptError, areg):
+            self.parser.eval("$cleanmulti()")
+        with self.assertRaisesRegex(ScriptError, areg):
+            self.parser.eval("$cleanmulti(foo,)")