Fix States/CommandDefinitions container semantics

__getitem__ now raises KeyError on missing keys (standard Python mapping
protocol). get() returns None for missing keys. __contains__ uses explicit
name comparison instead of delegating to __getitem__.

Author: iMicknl

pyoverkiz/models.py

@@ -454,19 +454,24 @@ def __iter__(self) -> Iterator[CommandDefinition]:
         """Iterate over defined commands."""
         return self._commands.__iter__()
 
-    def __contains__(self, name: str) -> bool:
+    def __contains__(self, name: object) -> bool:
         """Return True if a command with `name` exists."""
-        return self.__getitem__(name) is not None
+        return any(cd.command_name == name for cd in self._commands)
 
-    def __getitem__(self, command: str) -> CommandDefinition | None:
-        """Return the command definition or None if missing."""
-        return next((cd for cd in self._commands if cd.command_name == command), None)
+    def __getitem__(self, command: str) -> CommandDefinition:
+        """Return the command definition or raise KeyError if missing."""
+        result = next((cd for cd in self._commands if cd.command_name == command), None)
+        if result is None:
+            raise KeyError(command)
+        return result
 
     def __len__(self) -> int:
         """Return number of command definitions."""
         return len(self._commands)
 
-    get = __getitem__
+    def get(self, command: str) -> CommandDefinition | None:
+        """Return the command definition or None if missing."""
+        return next((cd for cd in self._commands if cd.command_name == command), None)
 
     def select(self, commands: list[str | OverkizCommand]) -> str | None:
         """Return the first command name that exists in this definition, or None."""
@@ -591,17 +596,20 @@ def __iter__(self) -> Iterator[State]:
         """Return an iterator over contained State objects."""
         return self._states.__iter__()
 
-    def __contains__(self, name: str) -> bool:
+    def __contains__(self, name: object) -> bool:
         """Return True if a state with the given name exists in the container."""
-        return self.__getitem__(name) is not None
+        return any(state.name == name for state in self._states)
 
-    def __getitem__(self, name: str) -> State | None:
-        """Return the State with the given name or None if missing."""
-        return next((state for state in self._states if state.name == name), None)
+    def __getitem__(self, name: str) -> State:
+        """Return the State with the given name or raise KeyError if missing."""
+        result = next((state for state in self._states if state.name == name), None)
+        if result is None:
+            raise KeyError(name)
+        return result
 
     def __setitem__(self, name: str, state: State) -> None:
         """Set or append a State identified by name."""
-        found = self.__getitem__(name)
+        found = self.get(name)
         if found is None:
             self._states.append(state)
         else:
@@ -611,12 +619,15 @@ def __len__(self) -> int:
         """Return number of states in the container."""
         return len(self._states)
 
-    get = __getitem__
+    def get(self, name: str) -> State | None:
+        """Return the State with the given name or None if missing."""
+        return next((state for state in self._states if state.name == name), None)
 
     def select(self, names: list[str]) -> State | None:
         """Return the first State that exists and has a non-None value, or None."""
         for name in names:
-            if (state := self[name]) and state.value is not None:
+            state = self.get(name)
+            if state is not None and state.value is not None:
                 return state
         return None
 

tests/test_models.py

@@ -442,6 +442,44 @@ def test_has_any_false(self):
         states = States(RAW_STATES)
         assert not states.has_any(["nonexistent", "also_nonexistent"])
 
+    def test_getitem_raises_keyerror_on_missing(self):
+        """Subscript access raises KeyError for missing states."""
+        states = States(RAW_STATES)
+        with pytest.raises(KeyError, match="nonexistent"):
+            states["nonexistent"]
+
+    def test_getitem_returns_state_on_hit(self):
+        """Subscript access returns the State for a known name."""
+        states = States(RAW_STATES)
+        state = states[STATE]
+        assert state.name == STATE
+
+    def test_contains_existing(self):
+        """'in' operator returns True for existing state names."""
+        states = States(RAW_STATES)
+        assert STATE in states
+
+    def test_contains_missing(self):
+        """'in' operator returns False for missing state names."""
+        states = States(RAW_STATES)
+        assert "nonexistent" not in states
+
+    def test_setitem_replaces_existing(self):
+        """Setting an existing state replaces it."""
+        states = States(RAW_STATES)
+        new_state = State(name=STATE, type=DataType.INTEGER, value=42)
+        states[STATE] = new_state
+        assert states.get(STATE).value == 42
+
+    def test_setitem_appends_new(self):
+        """Setting a new state appends it."""
+        states = States(RAW_STATES)
+        initial_len = len(states)
+        new_state = State(name="new:State", type=DataType.INTEGER, value=1)
+        states["new:State"] = new_state
+        assert len(states) == initial_len + 1
+        assert states.get("new:State").value == 1
+
 
 class TestCommandDefinitions:
     """Tests for CommandDefinitions container and helper methods."""
@@ -472,6 +510,33 @@ def test_has_any_false(self):
         cmds = CommandDefinitions([{"command_name": "close", "nparams": 0}])
         assert not cmds.has_any(["nonexistent", "also_nonexistent"])
 
+    def test_getitem_raises_keyerror_on_missing(self):
+        """Subscript access raises KeyError for missing commands."""
+        cmds = CommandDefinitions([{"command_name": "close", "nparams": 0}])
+        with pytest.raises(KeyError, match="nonexistent"):
+            cmds["nonexistent"]
+
+    def test_getitem_returns_command_on_hit(self):
+        """Subscript access returns the CommandDefinition for a known command."""
+        cmds = CommandDefinitions([{"command_name": "close", "nparams": 0}])
+        cmd = cmds["close"]
+        assert cmd.command_name == "close"
+
+    def test_get_returns_none_on_missing(self):
+        """get() returns None for missing commands."""
+        cmds = CommandDefinitions([{"command_name": "close", "nparams": 0}])
+        assert cmds.get("nonexistent") is None
+
+    def test_contains_existing(self):
+        """'in' operator returns True for existing command names."""
+        cmds = CommandDefinitions([{"command_name": "close", "nparams": 0}])
+        assert "close" in cmds
+
+    def test_contains_missing(self):
+        """'in' operator returns False for missing command names."""
+        cmds = CommandDefinitions([{"command_name": "close", "nparams": 0}])
+        assert "nonexistent" not in cmds
+
 
 class TestDefinition:
     """Tests for Definition model and its helper methods."""