better error messages for ListConfig errors

omry · Apr 17, 2020 · 622626b · 622626b
1 parent d8193f6
commit 622626b
Show file tree

Hide file tree

Showing 3 changed files with 25 additions and 14 deletions.
diff --git a/omegaconf/listconfig.py b/omegaconf/listconfig.py
@@ -24,12 +24,13 @@
 from .base import Container, ContainerMetadata, Node
 from .basecontainer import BaseContainer
 from .errors import (
+    ConfigAttributeError,
+    ConfigTypeError,
     ConfigValueError,
     KeyValidationError,
     MissingMandatoryValue,
     ReadonlyConfigError,
     ValidationError,
-    ConfigAttributeError,
 )
 from .nodes import ValueNode
 
@@ -66,7 +67,9 @@ def __init__(
 
     def _validate_get(self, key: Any, value: Any = None) -> None:
         if not isinstance(key, (int, slice)):
-            raise KeyValidationError("Invalid key type '$KEY_TYPE'")
+            raise KeyValidationError(
+                "ListConfig indices must be integers or slices, not $KEY_TYPE"
+            )
 
     def _validate_set(self, key: Any, value: Any) -> None:
 
@@ -287,8 +290,10 @@ def _get_node(
             if self._is_missing():
                 raise MissingMandatoryValue("Cannot get_node from a missing ListConfig")
             assert isinstance(self.__dict__["_content"], list)
+            if validate_access:
+                self._validate_get(key)
             return self.__dict__["_content"][key]  # type: ignore
-        except (IndexError, TypeError, MissingMandatoryValue) as e:
+        except (IndexError, TypeError, MissingMandatoryValue, KeyValidationError) as e:
             if validate_access:
                 self._format_and_raise(key=key, value=None, cause=e)
                 assert False
@@ -328,6 +333,11 @@ def pop(self, index: int = -1) -> Any:
             del self.__dict__["_content"][index]
             self._update_keys()
             return ret
+        except KeyValidationError as e:
+            self._format_and_raise(
+                key=index, value=None, cause=e, type_override=ConfigTypeError
+            )
+            assert False
         except Exception as e:
             self._format_and_raise(key=index, value=None, cause=e)
             assert False

diff --git a/tests/test_basic_ops_list.py b/tests/test_basic_ops_list.py
@@ -6,6 +6,7 @@
 
 from omegaconf import AnyNode, ListConfig, OmegaConf
 from omegaconf.errors import (
+    ConfigTypeError,
     KeyValidationError,
     MissingMandatoryValue,
     UnsupportedValueType,
@@ -108,8 +109,8 @@ def test_list_pop_on_unexpected_exception_not_modifying() -> None:
     src = [1, 2, 3, 4]
     c = OmegaConf.create(src)
 
-    with pytest.raises(Exception):
-        c.pop("foo")
+    with pytest.raises(ConfigTypeError):
+        c.pop("foo")  # type: ignore
     assert c == src
 
 

diff --git a/tests/test_errors.py b/tests/test_errors.py
@@ -490,10 +490,10 @@ def finalize(self, cfg: Any) -> None:
         Expected(
             create=lambda: OmegaConf.create([1, 2, 3]),
             op=lambda cfg: cfg._get_node("foo"),
-            exception_type=TypeError,
+            exception_type=KeyValidationError,
             key="foo",
             full_key="[foo]",
-            msg="list indices must be integers or slices, not str",
+            msg="ListConfig indices must be integers or slices, not str",
         ),
         id="list:get_nox_ex:invalid_index_type",
     ),
@@ -566,18 +566,18 @@ def finalize(self, cfg: Any) -> None:
             full_key="[0]",
             child_node=lambda cfg: cfg[0],
         ),
-        id="dict:readonly:pop",
+        id="list:readonly:pop",
     ),
     pytest.param(
         Expected(
-            create=lambda: create_readonly([1, 2, 3]),
+            create=lambda: OmegaConf.create([1, 2, 3]),
             op=lambda cfg: cfg.pop("Invalid_key_type"),
-            exception_type=ReadonlyConfigError,
-            msg="Cannot pop from read-only ListConfig",
+            exception_type=ConfigTypeError,
+            msg="ListConfig indices must be integers or slices, not str",
             key="Invalid_key_type",
             full_key="[Invalid_key_type]",
         ),
-        id="dict:readonly:pop",
+        id="list:pop_invalid_key",
     ),
     pytest.param(
         Expected(
@@ -642,7 +642,7 @@ def finalize(self, cfg: Any) -> None:
             create=lambda: OmegaConf.create([1, 2, 3]),
             op=lambda cfg: cfg.__getitem__("foo"),
             exception_type=KeyValidationError,
-            msg="Invalid key type 'str'",
+            msg="ListConfig indices must be integers or slices, not str",
             key="foo",
             full_key="[foo]",
         ),
@@ -676,7 +676,7 @@ def finalize(self, cfg: Any) -> None:
             create=lambda: OmegaConf.create([1, 2, 3]),
             op=lambda cfg: cfg.__setitem__("foo", 4),
             exception_type=KeyValidationError,
-            msg="Invalid key type 'str'",
+            msg="ListConfig indices must be integers or slices, not str",
             key="foo",
             full_key="[foo]",
         ),