Make getargs raise an error if the argument name is not found (#365)

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:

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):

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)