From 1b5fb35eaf7a582164b36bb39bb2bb7f718873e8 Mon Sep 17 00:00:00 2001 From: Ryan Ly Date: Tue, 26 May 2020 10:43:03 -0700 Subject: [PATCH] Make `getargs` raise an error if the argument name is not found (#365) --- src/hdmf/spec/spec.py | 7 +- src/hdmf/utils.py | 51 +++++++--- tests/unit/utils_test/test_docval.py | 142 ++++++++++++++++++++++++++- 3 files changed, 184 insertions(+), 16 deletions(-) diff --git a/src/hdmf/spec/spec.py b/src/hdmf/spec/spec.py index f5dd4c898..bf4252b3f 100644 --- a/src/hdmf/spec/spec.py +++ b/src/hdmf/spec/spec.py @@ -313,13 +313,12 @@ class BaseStorageSpec(Spec): @docval(*_attrbl_args) def __init__(self, **kwargs): - name, doc, parent, quantity, attributes, linkable, data_type_def, data_type_inc =\ - getargs('name', 'doc', 'parent', 'quantity', 'attributes', - 'linkable', 'data_type_def', 'data_type_inc', kwargs) + name, doc, quantity, attributes, linkable, data_type_def, data_type_inc =\ + getargs('name', 'doc', 'quantity', 'attributes', 'linkable', 'data_type_def', 'data_type_inc', kwargs) if name == NAME_WILDCARD and data_type_def is None and data_type_inc is None: raise ValueError("Cannot create Group or Dataset spec with wildcard name " "without specifying 'data_type_def' and/or 'data_type_inc'") - super().__init__(doc, name=name, parent=parent) + super().__init__(doc, name=name) default_name = getargs('default_name', kwargs) if default_name: if name is not None: diff --git a/src/hdmf/utils.py b/src/hdmf/utils.py index 6655a486a..448384b7b 100644 --- a/src/hdmf/utils.py +++ b/src/hdmf/utils.py @@ -634,31 +634,60 @@ def __googledoc(func, validator, returns=None, rtype=None): def getargs(*argnames): - '''getargs(*argnames, argdict) - Convenience function to retrieve arguments from a dictionary in batch - ''' + """getargs(*argnames, argdict) + Convenience function to retrieve arguments from a dictionary in batch. + + The last argument should be a dictionary, and the other arguments should be the keys (argument names) for which + to retrieve the values. + + :raises ValueError: if a argument name is not found in the dictionary or there is only one argument passed to this + function or the last argument is not a dictionary + :return: a single value if there is only one argument, or a list of values corresponding to the given argument names + """ if len(argnames) < 2: raise ValueError('Must supply at least one key and a dict') if not isinstance(argnames[-1], dict): - raise ValueError('last argument must be dict') + raise ValueError('Last argument must be a dict') kwargs = argnames[-1] if len(argnames) == 2: + if argnames[0] not in kwargs: + raise ValueError("Argument not found in dict: '%s'" % argnames[0]) return kwargs.get(argnames[0]) - return [kwargs.get(arg) for arg in argnames[:-1]] + ret = [] + for arg in argnames[:-1]: + if arg not in kwargs: + raise ValueError("Argument not found in dict: '%s'" % arg) + ret.append(kwargs.get(arg)) + return ret def popargs(*argnames): - '''popargs(*argnames, argdict) - Convenience function to retrieve and remove arguments from a dictionary in batch - ''' + """popargs(*argnames, argdict) + Convenience function to retrieve and remove arguments from a dictionary in batch. + + The last argument should be a dictionary, and the other arguments should be the keys (argument names) for which + to retrieve the values. + + :raises ValueError: if a argument name is not found in the dictionary or there is only one argument passed to this + function or the last argument is not a dictionary + :return: a single value if there is only one argument, or a list of values corresponding to the given argument names + """ if len(argnames) < 2: raise ValueError('Must supply at least one key and a dict') if not isinstance(argnames[-1], dict): - raise ValueError('last argument must be dict') + raise ValueError('Last argument must be a dict') kwargs = argnames[-1] if len(argnames) == 2: - return kwargs.pop(argnames[0]) - return [kwargs.pop(arg) for arg in argnames[:-1]] + try: + ret = kwargs.pop(argnames[0]) + except KeyError as ke: + raise ValueError('Argument not found in dict: %s' % str(ke)) + return ret + try: + ret = [kwargs.pop(arg) for arg in argnames[:-1]] + except KeyError as ke: + raise ValueError('Argument not found in dict: %s' % str(ke)) + return ret class ExtenderMeta(ABCMeta): diff --git a/tests/unit/utils_test/test_docval.py b/tests/unit/utils_test/test_docval.py index 5e753eccc..eba35db2a 100644 --- a/tests/unit/utils_test/test_docval.py +++ b/tests/unit/utils_test/test_docval.py @@ -1,6 +1,6 @@ import numpy as np -from hdmf.utils import docval, fmt_docval_args, get_docval, popargs, AllowPositional +from hdmf.utils import docval, fmt_docval_args, get_docval, getargs, popargs, AllowPositional from hdmf.testing import TestCase @@ -758,3 +758,143 @@ def test_shape_other_unpack_default(self): r"\(expected shape '\(None, 2\)'\)") with self.assertRaisesRegex(ValueError, err_msg): MyChainClass(self.obj1, [[100, 200], [300, 400], [500, 600]], arg4=obj2) + + +class TestGetargs(TestCase): + """Test the getargs function and its error conditions.""" + + def test_one_arg_first(self): + kwargs = {'a': 1, 'b': None} + expected_kwargs = kwargs.copy() + res = getargs('a', kwargs) + self.assertEqual(res, 1) + self.assertDictEqual(kwargs, expected_kwargs) + + def test_one_arg_second(self): + kwargs = {'a': 1, 'b': None} + expected_kwargs = kwargs.copy() + res = getargs('b', kwargs) + self.assertEqual(res, None) + self.assertDictEqual(kwargs, expected_kwargs) + + def test_many_args_get_some(self): + kwargs = {'a': 1, 'b': None, 'c': 3} + expected_kwargs = kwargs.copy() + + res = getargs('a', 'c', kwargs) + self.assertListEqual(res, [1, 3]) + self.assertDictEqual(kwargs, expected_kwargs) + + def test_many_args_get_all(self): + kwargs = {'a': 1, 'b': None, 'c': 3} + expected_kwargs = kwargs.copy() + + res = getargs('a', 'b', 'c', kwargs) + self.assertListEqual(res, [1, None, 3]) + self.assertDictEqual(kwargs, expected_kwargs) + + def test_many_args_reverse(self): + kwargs = {'a': 1, 'b': None, 'c': 3} + expected_kwargs = kwargs.copy() + res = getargs('c', 'b', 'a', kwargs) + self.assertListEqual(res, [3, None, 1]) + self.assertDictEqual(kwargs, expected_kwargs) + + def test_many_args_unpack(self): + kwargs = {'a': 1, 'b': None, 'c': 3} + expected_kwargs = kwargs.copy() + res1, res2, res3 = getargs('a', 'b', 'c', kwargs) + self.assertEqual(res1, 1) + self.assertEqual(res2, None) + self.assertEqual(res3, 3) + self.assertDictEqual(kwargs, expected_kwargs) + + def test_too_few_args(self): + kwargs = {'a': 1, 'b': None} + msg = 'Must supply at least one key and a dict' + with self.assertRaisesWith(ValueError, msg): + getargs(kwargs) + + def test_last_arg_not_dict(self): + kwargs = {'a': 1, 'b': None} + msg = 'Last argument must be a dict' + with self.assertRaisesWith(ValueError, msg): + getargs(kwargs, 'a') + + def test_arg_not_found_one_arg(self): + kwargs = {'a': 1, 'b': None} + msg = "Argument not found in dict: 'c'" + with self.assertRaisesWith(ValueError, msg): + getargs('c', kwargs) + + def test_arg_not_found_many_args(self): + kwargs = {'a': 1, 'b': None} + msg = "Argument not found in dict: 'c'" + with self.assertRaisesWith(ValueError, msg): + getargs('a', 'c', kwargs) + + +class TestPopargs(TestCase): + """Test the popargs function and its error conditions.""" + + def test_one_arg_first(self): + kwargs = {'a': 1, 'b': None} + res = popargs('a', kwargs) + self.assertEqual(res, 1) + self.assertDictEqual(kwargs, {'b': None}) + + def test_one_arg_second(self): + kwargs = {'a': 1, 'b': None} + res = popargs('b', kwargs) + self.assertEqual(res, None) + self.assertDictEqual(kwargs, {'a': 1}) + + def test_many_args_pop_some(self): + kwargs = {'a': 1, 'b': None, 'c': 3} + res = popargs('a', 'c', kwargs) + self.assertListEqual(res, [1, 3]) + self.assertDictEqual(kwargs, {'b': None}) + + def test_many_args_pop_all(self): + kwargs = {'a': 1, 'b': None, 'c': 3} + res = popargs('a', 'b', 'c', kwargs) + self.assertListEqual(res, [1, None, 3]) + self.assertDictEqual(kwargs, {}) + + def test_many_args_reverse(self): + kwargs = {'a': 1, 'b': None, 'c': 3} + res = popargs('c', 'b', 'a', kwargs) + self.assertListEqual(res, [3, None, 1]) + self.assertDictEqual(kwargs, {}) + + def test_many_args_unpack(self): + kwargs = {'a': 1, 'b': None, 'c': 3} + res1, res2, res3 = popargs('a', 'b', 'c', kwargs) + self.assertEqual(res1, 1) + self.assertEqual(res2, None) + self.assertEqual(res3, 3) + self.assertDictEqual(kwargs, {}) + + def test_too_few_args(self): + kwargs = {'a': 1, 'b': None} + msg = 'Must supply at least one key and a dict' + with self.assertRaisesWith(ValueError, msg): + popargs(kwargs) + + def test_last_arg_not_dict(self): + kwargs = {'a': 1, 'b': None} + msg = 'Last argument must be a dict' + with self.assertRaisesWith(ValueError, msg): + popargs(kwargs, 'a') + + def test_arg_not_found_one_arg(self): + kwargs = {'a': 1, 'b': None} + msg = "Argument not found in dict: 'c'" + with self.assertRaisesWith(ValueError, msg): + popargs('c', kwargs) + + def test_arg_not_found_many_args(self): + kwargs = {'a': 1, 'b': None} + msg = "Argument not found in dict: 'c'" + with self.assertRaisesWith(ValueError, msg): + popargs('a', 'c', kwargs)