Merge branch 'master' into refactor/deviceinfo_cli_output

miio/__init__.py

@@ -21,6 +21,16 @@
 # isort: on
 
 from miio.cloud import CloudDeviceInfo, CloudException, CloudInterface
+from miio.descriptorcollection import DescriptorCollection
+from miio.descriptors import (
+    AccessFlags,
+    ActionDescriptor,
+    Descriptor,
+    EnumDescriptor,
+    PropertyDescriptor,
+    RangeDescriptor,
+    ValidSettingRange,
+)
 from miio.devicefactory import DeviceFactory
 from miio.integrations.airdog.airpurifier import AirDogX3
 from miio.integrations.cgllc.airmonitor import AirQualityMonitor, AirQualityMonitorCGDN1

miio/descriptorcollection.py

@@ -23,7 +23,12 @@
 
 
 class DescriptorCollection(UserDict, Generic[T]):
-    """A container of descriptors."""
+    """A container of descriptors.
+
+    This is a glorified dictionary that provides several useful features for handling
+    descriptors like binding names (method_name, setter_name) to *device* callables,
+    setting property constraints, and handling duplicate identifiers.
+    """
 
     def __init__(self, *args, device: "Device"):
         self._device = device
@@ -60,11 +65,16 @@ def add_descriptor(self, descriptor: Descriptor):
         if not isinstance(descriptor, Descriptor):
             raise TypeError("Tried to add non-descriptor descriptor: %s", descriptor)
 
-        if descriptor.id in self.data:
-            descriptor.id = descriptor.id + "-2"
-            _LOGGER.warning("Appended '-2' to the id of %s", descriptor)
-            # TODO: append suffix to dupe ids
-            raise ValueError(f"Duplicate descriptor id: {descriptor.id}")
+        def _get_free_id(id_, suffix=2):
+            if id_ not in self.data:
+                return id_
+
+            while f"{id_}-{suffix}" in self.data:
+                suffix += 1
+
+            return f"{id_}-{suffix}"
+
+        descriptor.id = _get_free_id(descriptor.id)
 
         if isinstance(descriptor, PropertyDescriptor):
             self._handle_property_descriptor(descriptor)
@@ -82,15 +92,15 @@ def _handle_action_descriptor(self, prop: ActionDescriptor) -> None:
             prop.method = getattr(self._device, prop.method_name)
 
         if prop.method is None:
-            raise Exception(f"Neither method or method_name was defined for {prop}")
+            raise ValueError(f"Neither method or method_name was defined for {prop}")
 
     def _handle_property_descriptor(self, prop: PropertyDescriptor) -> None:
         """Bind the setter method to the property."""
         if prop.setter_name is not None:
             prop.setter = getattr(self._device, prop.setter_name)
 
         if prop.access & AccessFlags.Write and prop.setter is None:
-            raise Exception(f"Neither setter or setter_name was defined for {prop}")
+            raise ValueError(f"Neither setter or setter_name was defined for {prop}")
 
         self._handle_constraints(prop)
 
@@ -104,6 +114,11 @@ def _handle_constraints(self, prop: PropertyDescriptor) -> None:
                 )
                 prop.choices = retrieve_choices_function()
 
+            if prop.choices is None:
+                raise ValueError(
+                    f"Neither choices nor choices_attribute was defined for {prop}"
+                )
+
         elif prop.constraint == PropertyConstraint.Range:
             prop = cast(RangeDescriptor, prop)
             if prop.range_attribute is not None:

miio/device.py

@@ -4,7 +4,7 @@
 
 import click
 
-from .click_common import DeviceGroupMeta, LiteralParamType, command
+from .click_common import DeviceGroupMeta, LiteralParamType, command, format_output
 from .descriptorcollection import DescriptorCollection
 from .descriptors import AccessFlags, ActionDescriptor, Descriptor, PropertyDescriptor
 from .deviceinfo import DeviceInfo
@@ -167,6 +167,12 @@ def _initialize_descriptors(self) -> None:
         # Read descriptors from the status class
         self._descriptors.descriptors_from_object(self.status.__annotations__["return"])
 
+        if not self._descriptors:
+            _LOGGER.warning(
+                "'%s' does not specify any descriptors, please considering creating a PR.",
+                self.__class__.__name__,
+            )
+
         self._initialized = True
 
     @property
@@ -326,8 +332,13 @@ def supports_miot(self) -> bool:
     )
     def call_action(self, name: str, params=None):
         """Call action by name."""
-        act = self.actions()[name]
-        params = params or []
+        try:
+            act = self.actions()[name]
+        except KeyError:
+            raise ValueError("Unable to find action '%s'" % name)
+
+        if params is None:
+            return act.method()
 
         return act.method(params)
 
@@ -338,11 +349,12 @@ def call_action(self, name: str, params=None):
     )
     def change_setting(self, name: str, params=None):
         """Change setting value."""
-        setting = self.settings()[name]
-        params = params if params is not None else []
+        try:
+            setting = self.settings()[name]
+        except KeyError:
+            raise ValueError("Unable to find setting '%s'" % name)
 
-        if setting.access & AccessFlags.Write == 0:
-            raise ValueError("Property %s is not writable" % name)
+        params = params if params is not None else []
 
         return setting.setter(params)
 

miio/devicestatus.py

@@ -255,10 +255,6 @@ def decorator_setting(func):
 
         if setter is None and setter_name is None:
             raise Exception("setter_name needs to be defined")
-        if setter_name is None:
-            raise NotImplementedError(
-                "setter not yet implemented, use setter_name instead"
-            )
 
         common_values = {
             "id": qualified_name,

miio/integrations/genericmiot/genericmiot.py

@@ -60,10 +60,7 @@ def initialize_model(self):
     def status(self) -> GenericMiotStatus:
         """Return status based on the miot model."""
         properties = []
-        for _, prop in self.descriptors().sensors().items():
-            if prop.access & AccessFlags.Read == 0:
-                continue
-
+        for _, prop in self.sensors().items():
             extras = prop.extras
             prop = extras["miot_property"]
             q = {"siid": prop.siid, "piid": prop.piid, "did": prop.name}

miio/tests/test_descriptorcollection.py

@@ -0,0 +1,191 @@
+import pytest
+
+from miio import (
+    AccessFlags,
+    ActionDescriptor,
+    DescriptorCollection,
+    Device,
+    DeviceStatus,
+    EnumDescriptor,
+    PropertyDescriptor,
+    RangeDescriptor,
+    ValidSettingRange,
+)
+from miio.devicestatus import action, sensor, setting
+
+
+@pytest.fixture
+def dev(mocker):
+    d = Device("127.0.0.1", "68ffffffffffffffffffffffffffffff")
+    mocker.patch("miio.Device.send")
+    mocker.patch("miio.Device.send_handshake")
+    yield d
+
+
+def test_descriptors_from_device_object(mocker):
+    """Test descriptor collection from device class."""
+
+    class DummyDevice(Device):
+        @action(id="test", name="test")
+        def test_action(self):
+            pass
+
+    dev = DummyDevice("127.0.0.1", "68ffffffffffffffffffffffffffffff")
+    mocker.patch("miio.Device.send")
+    mocker.patch("miio.Device.send_handshake")
+
+    coll = DescriptorCollection(device=dev)
+    coll.descriptors_from_object(DummyDevice())
+    assert len(coll) == 1
+    assert isinstance(coll["test"], ActionDescriptor)
+
+
+def test_descriptors_from_status_object(dev):
+    coll = DescriptorCollection(device=dev)
+
+    class TestStatus(DeviceStatus):
+        @sensor(id="test", name="test sensor")
+        def test_sensor(self):
+            pass
+
+        @setting(id="test-setting", name="test setting", setter=lambda _: _)
+        def test_setting(self):
+            pass
+
+    status = TestStatus()
+    coll.descriptors_from_object(status)
+    assert len(coll) == 2
+    assert isinstance(coll["test"], PropertyDescriptor)
+    assert isinstance(coll["test-setting"], PropertyDescriptor)
+    assert coll["test-setting"].access & AccessFlags.Write
+
+
+@pytest.mark.parametrize(
+    "cls, params",
+    [
+        pytest.param(ActionDescriptor, {"method": lambda _: _}, id="action"),
+        pytest.param(PropertyDescriptor, {"status_attribute": "foo"}),
+    ],
+)
+def test_add_descriptor(dev: Device, cls, params):
+    """Test that adding a descriptor works."""
+    coll: DescriptorCollection = DescriptorCollection(device=dev)
+    coll.add_descriptor(cls(id="id", name="test name", **params))
+    assert len(coll) == 1
+    assert coll["id"] is not None
+
+
+def test_handle_action_descriptor(mocker, dev):
+    coll = DescriptorCollection(device=dev)
+    invalid_desc = ActionDescriptor(id="action", name="test name")
+    with pytest.raises(ValueError, match="Neither method or method_name was defined"):
+        coll.add_descriptor(invalid_desc)
+
+    mocker.patch.object(dev, "existing_method", create=True)
+
+    # Test method name binding
+    act_with_method_name = ActionDescriptor(
+        id="with-method-name", name="with-method-name", method_name="existing_method"
+    )
+    coll.add_descriptor(act_with_method_name)
+    assert act_with_method_name.method is not None
+
+    # Test non-existing method
+    act_with_method_name_missing = ActionDescriptor(
+        id="with-method-name-missing",
+        name="with-method-name-missing",
+        method_name="nonexisting_method",
+    )
+    with pytest.raises(AttributeError):
+        coll.add_descriptor(act_with_method_name_missing)
+
+
+def test_handle_writable_property_descriptor(mocker, dev):
+    coll = DescriptorCollection(device=dev)
+    data = {
+        "name": "",
+        "status_attribute": "",
+        "access": AccessFlags.Write,
+    }
+    invalid = PropertyDescriptor(id="missing_setter", **data)
+    with pytest.raises(ValueError, match="Neither setter or setter_name was defined"):
+        coll.add_descriptor(invalid)
+
+    mocker.patch.object(dev, "existing_method", create=True)
+
+    # Test name binding
+    setter_name_desc = PropertyDescriptor(
+        **data, id="setter_name", setter_name="existing_method"
+    )
+    coll.add_descriptor(setter_name_desc)
+    assert setter_name_desc.setter is not None
+
+    with pytest.raises(AttributeError):
+        coll.add_descriptor(
+            PropertyDescriptor(
+                **data, id="missing_setter", setter_name="non_existing_setter"
+            )
+        )
+
+
+def test_handle_enum_constraints(dev, mocker):
+    coll = DescriptorCollection(device=dev)
+
+    data = {
+        "name": "enum",
+        "status_attribute": "attr",
+    }
+
+    mocker.patch.object(dev, "choices_attr", create=True)
+
+    # Check that error is raised if choices are missing
+    invalid = EnumDescriptor(id="missing", **data)
+    with pytest.raises(
+        ValueError, match="Neither choices nor choices_attribute was defined"
+    ):
+        coll.add_descriptor(invalid)
+
+    # Check that binding works
+    choices_attribute = EnumDescriptor(
+        id="with_choices_attr", choices_attribute="choices_attr", **data
+    )
+    coll.add_descriptor(choices_attribute)
+    assert len(coll) == 1
+    assert coll["with_choices_attr"].choices is not None
+
+
+def test_handle_range_constraints(dev, mocker):
+    coll = DescriptorCollection(device=dev)
+
+    data = {
+        "name": "name",
+        "status_attribute": "attr",
+        "min_value": 0,
+        "max_value": 100,
+        "step": 1,
+    }
+
+    # Check regular descriptor
+    desc = RangeDescriptor(id="regular", **data)
+    coll.add_descriptor(desc)
+    assert coll["regular"].max_value == 100
+
+    mocker.patch.object(dev, "range", create=True, new=ValidSettingRange(-1, 1000, 10))
+    range_attr = RangeDescriptor(id="range_attribute", range_attribute="range", **data)
+    coll.add_descriptor(range_attr)
+
+    assert coll["range_attribute"].min_value == -1
+    assert coll["range_attribute"].max_value == 1000
+    assert coll["range_attribute"].step == 10
+
+
+def test_duplicate_identifiers(dev):
+    coll = DescriptorCollection(device=dev)
+    for i in range(3):
+        coll.add_descriptor(
+            ActionDescriptor(id="action", name=f"action {i}", method=lambda _: _)
+        )
+
+    assert coll["action"]
+    assert coll["action-2"]
+    assert coll["action-3"]