From 63fa1633f1d05fec7d44cc895d52b6abb845c95c Mon Sep 17 00:00:00 2001 From: Bob Swift Date: Thu, 6 Jan 2022 10:43:47 -0700 Subject: [PATCH 1/4] Allow empty strings in multi-value variables - Keep empty strings in multi-value lists - Add scripting function to remove empty strings from multi-value - Update tests --- picard/metadata.py | 5 ++-- picard/script/functions.py | 21 ++++++++++++++ test/test_script.py | 59 ++++++++++++++++++++++++++++++++++++-- 3 files changed, 80 insertions(+), 5 deletions(-) diff --git a/picard/metadata.py b/picard/metadata.py index dca8ffb8a..180da615a 100644 --- a/picard/metadata.py +++ b/picard/metadata.py @@ -433,8 +433,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: diff --git a/picard/script/functions.py b/picard/script/functions.py index 091a1a023..93c3a4d10 100644 --- a/picard/script/functions.py +++ b/picard/script/functions.py @@ -1494,3 +1494,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_( + """`$clean_multi(name)` + +Removes all empty string elements from the multi-value variable. + +Example: + + $$setmulti(test,one; ; two; three) + $clean_multi(%test%) + +Result: Sets the value of 'test' to ["one", "two", "three"]. + +_Since Picard 2.8_""" +)) +def func_clean_multi(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 "" diff --git a/test/test_script.py b/test/test_script.py index 49d18bb7e..f72714360 100644 --- a/test/test_script.py +++ b/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() @@ -1785,3 +1785,56 @@ class ScriptParserTest(PicardTestCase): self.parser.eval("$is_multi()") with self.assertRaisesRegex(ScriptError, areg): self.parser.eval("$is_multi(a,)") + + def test_cmd_clean_multi(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("$clean_multi(foo)%foo%", "one; two", context) + self.assertScriptResultEquals("$clean_multi(bar)%bar%", "one; two", context) + self.assertScriptResultEquals("$clean_multi(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("$clean_multi(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("$clean_multi(%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("$clean_multi(foo)%foo%", "one", context) + self.assertScriptResultEquals("$clean_multi(bar)%bar%", "one; ; two", context) + self.assertScriptResultEquals("$clean_multi(baz)%baz%", "", context) + + # Tests with invalid number of arguments + areg = r"^\d+:\d+:\$clean_multi: Wrong number of arguments for \$clean_multi: Expected exactly 1, " + with self.assertRaisesRegex(ScriptError, areg): + self.parser.eval("$clean_multi()") + with self.assertRaisesRegex(ScriptError, areg): + self.parser.eval("$clean_multi(foo,)") From 49c6c0f2ec8fd7808c667600f70330b155bd55b7 Mon Sep 17 00:00:00 2001 From: Bob Swift Date: Fri, 7 Jan 2022 08:56:45 -0700 Subject: [PATCH 2/4] Remove underscore from clean_multi for consistency with other functions --- picard/script/functions.py | 6 +++--- test/test_script.py | 24 ++++++++++++------------ 2 files changed, 15 insertions(+), 15 deletions(-) diff --git a/picard/script/functions.py b/picard/script/functions.py index 93c3a4d10..94b1ad2af 100644 --- a/picard/script/functions.py +++ b/picard/script/functions.py @@ -1497,20 +1497,20 @@ def func_is_multi(parser, multi): @script_function(eval_args=True, documentation=N_( - """`$clean_multi(name)` + """`$cleanmulti(name)` Removes all empty string elements from the multi-value variable. Example: $$setmulti(test,one; ; two; three) - $clean_multi(%test%) + $cleanmulti(%test%) Result: Sets the value of 'test' to ["one", "two", "three"]. _Since Picard 2.8_""" )) -def func_clean_multi(parser, multi): +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 diff --git a/test/test_script.py b/test/test_script.py index f72714360..6d9719d88 100644 --- a/test/test_script.py +++ b/test/test_script.py @@ -1786,7 +1786,7 @@ class ScriptParserTest(PicardTestCase): with self.assertRaisesRegex(ScriptError, areg): self.parser.eval("$is_multi(a,)") - def test_cmd_clean_multi(self): + def test_cmd_cleanmulti(self): context = Metadata() context["foo"] = ["", "one", "two"] context["bar"] = ["one", "", "two"] @@ -1797,9 +1797,9 @@ class ScriptParserTest(PicardTestCase): self.assertScriptResultEquals("%bar%", "one; ; two", context) self.assertScriptResultEquals("%baz%", "one; two; ", context) # Test cleaned values - self.assertScriptResultEquals("$clean_multi(foo)%foo%", "one; two", context) - self.assertScriptResultEquals("$clean_multi(bar)%bar%", "one; two", context) - self.assertScriptResultEquals("$clean_multi(baz)%baz%", "one; two", context) + 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"] = ["", "", ""] @@ -1807,7 +1807,7 @@ class ScriptParserTest(PicardTestCase): # Confirm initial values self.assertScriptResultEquals("%foo%", "; ; ", context) # Test cleaned values - self.assertScriptResultEquals("$clean_multi(foo)%foo%", "", context) + self.assertScriptResultEquals("$cleanmulti(foo)%foo%", "", context) # Test clean with indirect argument context["foo"] = ["", "one", "two"] @@ -1816,7 +1816,7 @@ class ScriptParserTest(PicardTestCase): # Confirm initial values self.assertScriptResultEquals("%foo%", "; one; two", context) # Test cleaned values - self.assertScriptResultEquals("$clean_multi(%bar%)%foo%", "one; two", context) + self.assertScriptResultEquals("$cleanmulti(%bar%)%foo%", "one; two", context) # Test clean with non-multi argument context["foo"] = "one" @@ -1828,13 +1828,13 @@ class ScriptParserTest(PicardTestCase): self.assertScriptResultEquals("%bar%", "one; ; two", context) self.assertScriptResultEquals("%baz%", "", context) # Test cleaned values - self.assertScriptResultEquals("$clean_multi(foo)%foo%", "one", context) - self.assertScriptResultEquals("$clean_multi(bar)%bar%", "one; ; two", context) - self.assertScriptResultEquals("$clean_multi(baz)%baz%", "", context) + 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+:\$clean_multi: Wrong number of arguments for \$clean_multi: Expected exactly 1, " + areg = r"^\d+:\d+:\$cleanmulti: Wrong number of arguments for \$cleanmulti: Expected exactly 1, " with self.assertRaisesRegex(ScriptError, areg): - self.parser.eval("$clean_multi()") + self.parser.eval("$cleanmulti()") with self.assertRaisesRegex(ScriptError, areg): - self.parser.eval("$clean_multi(foo,)") + self.parser.eval("$cleanmulti(foo,)") From d0f512f79fded35ad5d32fc76ee10cbd981f3dd4 Mon Sep 17 00:00:00 2001 From: Bob Swift Date: Fri, 7 Jan 2022 09:30:58 -0700 Subject: [PATCH 3/4] Additional tests to ensure that changes don't break other functions --- test/test_script.py | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/test/test_script.py b/test/test_script.py index 6d9719d88..dde0e1645 100644 --- a/test/test_script.py +++ b/test/test_script.py @@ -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) @@ -926,6 +929,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) From 17786e00c7071d9640cee5601dc6c84a85952525 Mon Sep 17 00:00:00 2001 From: Bob Swift Date: Fri, 7 Jan 2022 09:41:15 -0700 Subject: [PATCH 4/4] Fix typos in function documentation. --- picard/script/functions.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/picard/script/functions.py b/picard/script/functions.py index 94b1ad2af..463c78395 100644 --- a/picard/script/functions.py +++ b/picard/script/functions.py @@ -1503,8 +1503,8 @@ Removes all empty string elements from the multi-value variable. Example: - $$setmulti(test,one; ; two; three) - $cleanmulti(%test%) + $setmulti(test,one; ; two; three) + $cleanmulti(test) Result: Sets the value of 'test' to ["one", "two", "three"].