variable: Fix type-related issues with desc property (#667)

* variable: Value description in log message only for integers.

* Raise TypeError when requesting value description of non-int data.

According to the type hints, the mapping mechanism was only ever
intended for integers (emulating an enumeration).  Avoids a later
DictionaryError for the missing key and some type checker errors
because decode_desc() shall only accept int.

Add the same check in the write()-based variable access API.

* Style fix (black).

Better overview if not on one line.

* Avoid else after raise.

Simplify by using the walrus operator.

* Extend test coverage for ODVariable.

* Extend test coverage for Variable.

Author: acolomb

canopen/objectdictionary/__init__.py

@@ -500,11 +500,10 @@ def encode_phys(self, value: Union[int, bool, float, str, bytes]) -> int:
     def decode_desc(self, value: int) -> str:
         if not self.value_descriptions:
             raise ObjectDictionaryError("No value descriptions exist")
-        elif value not in self.value_descriptions:
+        elif (desc := self.value_descriptions.get(value)) is None:
             raise ObjectDictionaryError(
                 f"No value description exists for {value}")
-        else:
-            return self.value_descriptions[value]
+        return desc
 
     def encode_desc(self, desc: str) -> int:
         if not self.value_descriptions:

canopen/variable.py

@@ -77,8 +77,11 @@ def raw(self) -> Union[int, bool, float, str, bytes]:
         """
         value = self.od.decode_raw(self.data)
         text = f"Value of {self.name!r} ({pretty_index(self.index, self.subindex)}) is {value!r}"
-        if value in self.od.value_descriptions:
-            text += f" ({self.od.value_descriptions[value]})"
+        if (
+            isinstance(value, int)
+            and (desc := self.od.value_descriptions.get(value)) is not None
+        ):
+            text += f" ({desc})"
         logger.debug(text)
         return value
 
@@ -108,8 +111,14 @@ def phys(self, value: Union[int, bool, float, str, bytes]):
 
     @property
     def desc(self) -> str:
-        """Converts to and from a description of the value as a string."""
-        value = self.od.decode_desc(self.raw)
+        """Convert to and from a description of the value as a string.
+
+        :raises TypeError: If the received raw data was anything but an integer value.
+        """
+        raw_int = self.raw
+        if not isinstance(raw_int, int):
+            raise TypeError("Description of values only supported for integer objects")
+        value = self.od.decode_desc(raw_int)
         logger.debug("Description is '%s'", value)
         return value
 
@@ -146,7 +155,9 @@ def read(self, fmt: str = "raw") -> Union[int, bool, float, str, bytes]:
         raise ValueError(f"Invalid format '{fmt}'")
 
     def write(
-        self, value: Union[int, bool, float, str, bytes], fmt: str = "raw"
+        self,
+        value: Union[int, bool, float, str, bytes],
+        fmt: str = "raw",
     ) -> None:
         """Alternative way of writing using a function instead of attributes.
 
@@ -157,12 +168,15 @@ def write(
              - 'raw'
              - 'phys'
              - 'desc'
+        :raises TypeError: If the "desc" format was specified with anything but a string value.
         """
         if fmt == "raw":
             self.raw = value
         elif fmt == "phys":
             self.phys = value
         elif fmt == "desc":
+            if not isinstance(value, str):
+                raise TypeError("fmt=desc requires a string value")
             self.desc = value
 
 

test/test_od.py

@@ -221,13 +221,23 @@ def test_phys_float_factor_decodes_to_float(self):
     def test_desc(self):
         var = od.ODVariable("Test UNSIGNED8", 0x1000)
         var.data_type = od.UNSIGNED8
+        with self.assertRaises(od.ObjectDictionaryError):
+            var.decode_desc(0)
+        with self.assertRaises(od.ObjectDictionaryError):
+            var.encode_desc("")
+
         var.add_value_description(0, "Value 0")
         var.add_value_description(1, "Value 1")
         var.add_value_description(3, "Value 3")
 
         self.assertEqual(var.decode_desc(0), "Value 0")
         self.assertEqual(var.decode_desc(3), "Value 3")
+        with self.assertRaises(od.ObjectDictionaryError):
+            var.decode_desc(2)
+
         self.assertEqual(var.encode_desc("Value 1"), 1)
+        with self.assertRaises(ValueError):
+            var.encode_desc("UNDEFINED")
 
     def test_bits(self):
         var = od.ODVariable("Test UNSIGNED8", 0x1000)

test/test_variable.py

@@ -37,15 +37,20 @@ def test_write_desc(self):
         v = _StubVariable(var)
         v.write("On", fmt="desc")
         self.assertEqual(v.raw, 1)
+        self.assertEqual(v.desc, "On")
+        with self.assertRaises(TypeError):
+            v.write(b"", fmt="desc")
 
     def test_raw_with_string_value(self):
         var = od.ODVariable("Test VISIBLE_STRING", 0x1000)
         var.data_type = od.VISIBLE_STRING
         var.default = "hello"
         var.add_value_description(0, "Off")
         v = _StubVariable(var)
-        # String value must not be looked up in value_descriptions
         self.assertEqual(v.raw, "hello")
+        # String value must not be looked up in value_descriptions
+        with self.assertRaises(TypeError):
+            _ = v.desc
 
     def test_bits(self):
         var = od.ODVariable("Test UNSIGNED8", 0x1000)