From 25f6a60fe5b78b812ddbda8f975a5fd3362ff9f1 Mon Sep 17 00:00:00 2001 From: Laurent Monin Date: Fri, 19 Apr 2019 09:22:53 +0200 Subject: [PATCH 1/3] Use argcount since it's equal to len(args) --- picard/script.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/picard/script.py b/picard/script.py index f7520abb5..254e540e5 100644 --- a/picard/script.py +++ b/picard/script.py @@ -94,7 +94,7 @@ class ScriptFunction(object): argcount = len(args) if argnum_bound and not (argnum_bound.lower <= argcount and (argnum_bound.upper is None - or len(args) <= argnum_bound.upper)): + or argcount <= argnum_bound.upper)): raise ScriptError( "Wrong number of arguments for $%s: Expected %s, got %i at position %i, line %i" % (name, From 437c55ea2e7219f7b0962d41311fb7519e2f4fea Mon Sep 17 00:00:00 2001 From: Laurent Monin Date: Fri, 19 Apr 2019 10:05:21 +0200 Subject: [PATCH 2/3] Make wrong number of arguments script error message more explicit, along tests --- picard/script.py | 29 +++++++++++++++++------------ test/test_script.py | 39 +++++++++++++++++++++++++++++++++++++++ 2 files changed, 56 insertions(+), 12 deletions(-) diff --git a/picard/script.py b/picard/script.py index 254e540e5..139ce688e 100644 --- a/picard/script.py +++ b/picard/script.py @@ -92,18 +92,23 @@ class ScriptFunction(object): try: argnum_bound = parser.functions[name].argcount argcount = len(args) - if argnum_bound and not (argnum_bound.lower <= argcount - and (argnum_bound.upper is None - or argcount <= argnum_bound.upper)): - raise ScriptError( - "Wrong number of arguments for $%s: Expected %s, got %i at position %i, line %i" - % (name, - str(argnum_bound.lower) - if argnum_bound.upper is None - else "%i - %i" % (argnum_bound.lower, argnum_bound.upper), - argcount, - parser._x, - parser._y)) + if argnum_bound: + too_few_args = argcount < argnum_bound.lower + if argnum_bound.upper is not None: + if argnum_bound.lower == argnum_bound.upper: + expected = "exactly %i" % argnum_bound.lower + else: + expected = "between %i and %i" % (argnum_bound.lower, argnum_bound.upper) + too_many_args = argcount > argnum_bound.upper + else: + expected = "at least %i" % argnum_bound.lower + too_many_args = False + + if too_few_args or too_many_args: + raise ScriptError( + "Wrong number of arguments for $%s: Expected %s, got %i at position %i, line %i" + % (name, expected, argcount, parser._x, parser._y) + ) except KeyError: raise ScriptUnknownFunction("Unknown function '%s'" % name) diff --git a/test/test_script.py b/test/test_script.py index d3eddd5d8..e982fca51 100644 --- a/test/test_script.py +++ b/test/test_script.py @@ -390,6 +390,45 @@ class ScriptParserTest(PicardTestCase): except ScriptError: self.fail("Function noargs raised ScriptError unexpectedly.") + def test_cmd_with_wrong_argcount_or(self): + # $or() requires at least 2 arguments + with self.assertRaises(ScriptError): + self.parser.eval('$or(0)') + try: + self.parser.eval('$or(0,1)') + except ScriptError as why: + self.fail("Function raised ScriptError: %s" % why) + try: + self.parser.eval('$or(0,1,1,1,1)') + except ScriptError as why: + self.fail("Function raised ScriptError: %s" % why) + + def test_cmd_with_wrong_argcount_eq(self): + # $eq() requires exactly 2 arguments + with self.assertRaises(ScriptError): + self.parser.eval('$eq(0)') + with self.assertRaises(ScriptError): + self.parser.eval('$eq(0,0,0)') + try: + self.parser.eval('$eq(1,1)') + except ScriptError as why: + self.fail("Function raised ScriptError: %s" % why) + + def test_cmd_with_wrong_argcount_if(self): + # $if() requires 2 or 3 arguments + with self.assertRaises(ScriptError): + self.parser.eval('$if(1)') + with self.assertRaises(ScriptError): + self.parser.eval('$if(1,a,b,c)') + try: + self.parser.eval('$if(1,a)') + except ScriptError as why: + self.fail("Function raised ScriptError: %s" % why) + try: + self.parser.eval('$if(1,a,b)') + except ScriptError as why: + self.fail("Function raised ScriptError: %s" % why) + def test_cmd_unset_simple(self): context = Metadata() context['title'] = u'Foo' From 2848b76f1c9f522deaf38d9e61e331c3af243258 Mon Sep 17 00:00:00 2001 From: Laurent Monin Date: Thu, 25 Apr 2019 14:14:45 +0200 Subject: [PATCH 3/3] Improve script tests: use assertRaisesRegex() and remove useless self.fail() Suggested by Mineo --- test/test_script.py | 38 +++++++++----------------------------- 1 file changed, 9 insertions(+), 29 deletions(-) diff --git a/test/test_script.py b/test/test_script.py index e982fca51..136040b20 100644 --- a/test/test_script.py +++ b/test/test_script.py @@ -385,49 +385,29 @@ class ScriptParserTest(PicardTestCase): self.assertEqual(result, u'artist/title') def test_cmd_with_not_arguments(self): - try: - self.parser.eval("$noargstest()") - except ScriptError: - self.fail("Function noargs raised ScriptError unexpectedly.") + self.parser.eval("$noargstest()") def test_cmd_with_wrong_argcount_or(self): # $or() requires at least 2 arguments - with self.assertRaises(ScriptError): + areg = r"^Wrong number of arguments for \$or: Expected at least 2, " + with self.assertRaisesRegex(ScriptError, areg): self.parser.eval('$or(0)') - try: - self.parser.eval('$or(0,1)') - except ScriptError as why: - self.fail("Function raised ScriptError: %s" % why) - try: - self.parser.eval('$or(0,1,1,1,1)') - except ScriptError as why: - self.fail("Function raised ScriptError: %s" % why) def test_cmd_with_wrong_argcount_eq(self): # $eq() requires exactly 2 arguments - with self.assertRaises(ScriptError): + areg = r"^Wrong number of arguments for \$eq: Expected exactly 2, " + with self.assertRaisesRegex(ScriptError, areg): self.parser.eval('$eq(0)') - with self.assertRaises(ScriptError): + with self.assertRaisesRegex(ScriptError, areg): self.parser.eval('$eq(0,0,0)') - try: - self.parser.eval('$eq(1,1)') - except ScriptError as why: - self.fail("Function raised ScriptError: %s" % why) def test_cmd_with_wrong_argcount_if(self): + areg = r"^Wrong number of arguments for \$if: Expected between 2 and 3, " # $if() requires 2 or 3 arguments - with self.assertRaises(ScriptError): + with self.assertRaisesRegex(ScriptError, areg): self.parser.eval('$if(1)') - with self.assertRaises(ScriptError): + with self.assertRaisesRegex(ScriptError, areg): self.parser.eval('$if(1,a,b,c)') - try: - self.parser.eval('$if(1,a)') - except ScriptError as why: - self.fail("Function raised ScriptError: %s" % why) - try: - self.parser.eval('$if(1,a,b)') - except ScriptError as why: - self.fail("Function raised ScriptError: %s" % why) def test_cmd_unset_simple(self): context = Metadata()