From d2ce2d20d4b96b7feca7af6b0ccbf50968f844f8 Mon Sep 17 00:00:00 2001 From: Ryan Nitcher Date: Thu, 21 Nov 2024 23:05:09 -0700 Subject: [PATCH] Remove Custom Parsers - Fix: Back out custom value parser logic. - Fix: Remove overloaded range set functionality. - Fix: Improve error messages when user forgets quotes around cli arguments. --- kasa/cli/feature.py | 22 +++++-- kasa/feature.py | 25 +------ kasa/iot/modules/motion.py | 109 ++++++++++--------------------- tests/iot/modules/test_motion.py | 50 +++++++------- tests/test_feature.py | 5 +- 5 files changed, 84 insertions(+), 127 deletions(-) diff --git a/kasa/cli/feature.py b/kasa/cli/feature.py index 35dba781..a4c739f6 100644 --- a/kasa/cli/feature.py +++ b/kasa/cli/feature.py @@ -6,10 +6,7 @@ import ast import asyncclick as click -from kasa import ( - Device, - Feature, -) +from kasa import Device, Feature from .common import ( echo, @@ -133,7 +130,22 @@ async def feature( echo(f"{feat.name} ({name}): {feat.value}{unit}") return feat.value - value = feat.parse_value(value, ast.literal_eval) + try: + # Attempt to parse as python literal. + value = ast.literal_eval(value) + except ValueError: + # The value is probably an unquoted string, so we'll raise an error, + # and tell the user to quote the string. + raise click.exceptions.BadParameter( + f'{repr(value)} for {name} (Perhaps you forgot to "quote" the value?)' + ) from SyntaxError + except SyntaxError: + # There are likely miss-matched quotes or odd characters in the input, + # so abort and complain to the user. + raise click.exceptions.BadParameter( + f"{repr(value)} for {name}" + ) from SyntaxError + echo(f"Changing {name} from {feat.value} to {value}") response = await dev.features[name].set_value(value) await dev.update() diff --git a/kasa/feature.py b/kasa/feature.py index 3b194ed4..f88221c4 100644 --- a/kasa/feature.py +++ b/kasa/feature.py @@ -163,9 +163,6 @@ class Feature: #: If set, this property will be used to get *choices*. choices_getter: str | Callable[[], list[str]] | None = None - #: Value converter, for when working with complex types. - value_parser: str | None = None - def __post_init__(self) -> None: """Handle late-binding of members.""" # Populate minimum & maximum values, if range_getter is given @@ -258,7 +255,7 @@ class Feature: elif self.type == Feature.Type.Choice: # noqa: SIM102 if not self.choices or value not in self.choices: raise ValueError( - f"Unexpected value for {self.name}: {value}" + f"Unexpected value for {self.name}: '{value}'" f" - allowed: {self.choices}" ) @@ -273,26 +270,6 @@ class Feature: return await attribute_setter(value) - def parse_value( - self, value: str, fallback: Callable[[str], Any | None] = lambda x: None - ) -> Any | None: - """Attempt to parse a given string into a value accepted by this feature.""" - parser = self._get_property_value(self.value_parser) - parser = parser if parser else fallback - allowed = f"{self.choices}" if self.choices else "Unknown" - try: - parsed = parser(value) - if parsed is None: - raise ValueError( - f"Unexpected value for {self.name}: {value}" - f" - allowed: {allowed}" - ) - return parsed - except SyntaxError as se: - raise ValueError( - f"{se.msg} for {self.name}: {value}" f" - allowed: {allowed}", - ) from se - def __repr__(self) -> str: try: value = self.value diff --git a/kasa/iot/modules/motion.py b/kasa/iot/modules/motion.py index 9b7d3b51..98404cd5 100644 --- a/kasa/iot/modules/motion.py +++ b/kasa/iot/modules/motion.py @@ -5,7 +5,6 @@ from __future__ import annotations import logging import math from enum import Enum -from typing import Literal, overload from ...exceptions import KasaException from ...feature import Feature @@ -69,7 +68,6 @@ class Motion(IotModule): attribute_setter="_set_range_cli", type=Feature.Type.Choice, choices_getter="ranges", - value_parser="parse_range_value", category=Feature.Category.Config, ) ) @@ -108,7 +106,7 @@ class Motion(IotModule): device=self._device, container=self, id="pir_value", - name="PIR Reading", + name="PIR Value", icon="mdi:motion-sensor", attribute_getter="pir_value", attribute_setter=None, @@ -117,21 +115,6 @@ class Motion(IotModule): ) ) - self._add_feature( - Feature( - device=self._device, - container=self, - id="pir_percent", - name="PIR Percentage", - icon="mdi:motion-sensor", - attribute_getter="pir_percent", - attribute_setter=None, - type=Feature.Type.Sensor, - category=Feature.Category.Info, - unit_getter=lambda: "%", - ) - ) - self._add_feature( Feature( device=self._device, @@ -188,6 +171,21 @@ class Motion(IotModule): ) ) + self._add_feature( + Feature( + device=self._device, + container=self, + id="pir_percent", + name="PIR Percentile", + icon="mdi:motion-sensor", + attribute_getter="pir_percent", + attribute_setter=None, + type=Feature.Type.Sensor, + category=Feature.Category.Debug, + unit_getter=lambda: "%", + ) + ) + def query(self) -> dict: """Request PIR configuration.""" req = merge( @@ -233,33 +231,15 @@ class Motion(IotModule): """Enable/disable PIR.""" return await self.call("set_enable", {"enable": int(state)}) - def _parse_range_value(self, value: str) -> int | Range | None: - """Attempt to parse a range value from the given string.""" - _LOGGER.debug("Parse Range Value: %s", value) - parsed: int | Range | None = None - try: - parsed = int(value) - _LOGGER.debug("Parse Range Value: %s is an integer.", value) - return parsed - except ValueError: - _LOGGER.debug("Parse Range Value: %s is not an integer.", value) - value = value.strip().upper() - if value in Range._member_names_: - _LOGGER.debug("Parse Range Value: %s is an enumeration.", value) - parsed = Range[value] - return parsed - _LOGGER.debug("Parse Range Value: %s is not a Range Value.", value) - return None - @property - def ranges(self) -> list[Range]: + def ranges(self) -> list[str]: """Return set of supported range classes.""" range_min = 0 range_max = len(self.config["array"]) valid_ranges = list() for r in Range: if (r.value >= range_min) and (r.value < range_max): - valid_ranges.append(r) + valid_ranges.append(r.name) return valid_ranges @property @@ -267,45 +247,27 @@ class Motion(IotModule): """Return motion detection Range.""" return Range(self.config["trigger_index"]) - @overload - async def set_range(self, *, range: Range) -> dict: ... - - @overload - async def set_range(self, *, range: Literal[Range.Custom], value: int) -> dict: ... - - @overload - async def set_range(self, *, value: int) -> dict: ... - - async def set_range( - self, *, range: Range | None = None, value: int | None = None - ) -> dict: + async def set_range(self, range: Range) -> dict: """Set the Range for the sensor. - :param Range: for using standard Ranges - :param custom_Range: Range in decimeters, overrides the Range parameter + :param Range: the range class to use. """ - if value is not None: - if range is not None and range is not Range.Custom: - raise KasaException( - f"Refusing to set non-custom range {range} to value {value}." - ) - elif value is None: - raise KasaException("Custom range threshold may not be set to None.") - payload = {"index": Range.Custom.value, "value": value} - elif range is not None: - payload = {"index": range.value} - else: - raise KasaException("Either range or value needs to be defined") - + payload = {"index": range.value} return await self.call("set_trigger_sens", payload) - async def _set_range_cli(self, input: Range | int) -> dict: - if isinstance(input, Range): - return await self.set_range(range=input) - elif isinstance(input, int): - return await self.set_range(value=input) - else: - raise KasaException(f"Invalid type: {type(input)} given to cli motion set.") + def _parse_range_value(self, value: str) -> Range: + """Attempt to parse a range value from the given string.""" + value = value.strip().capitalize() + if value not in Range._member_names_: + raise KasaException( + f"Invalid range value: '{value}'." + f" Valid options are: {Range._member_names_}" + ) + return Range[value] + + async def _set_range_cli(self, input: str) -> dict: + value = self._parse_range_value(input) + return await self.set_range(range=value) def get_range_threshold(self, range_type: Range) -> int: """Get the distance threshold at which the PIR sensor is will trigger.""" @@ -322,7 +284,8 @@ class Motion(IotModule): async def set_threshold(self, value: int) -> dict: """Set the distance threshold at which the PIR sensor is will trigger.""" - return await self.set_range(value=value) + payload = {"index": Range.Custom.value, "value": value} + return await self.call("set_trigger_sens", payload) @property def inactivity_timeout(self) -> int: diff --git a/tests/iot/modules/test_motion.py b/tests/iot/modules/test_motion.py index 723557f5..8f53c664 100644 --- a/tests/iot/modules/test_motion.py +++ b/tests/iot/modules/test_motion.py @@ -1,8 +1,6 @@ -import pytest from pytest_mock import MockerFixture from kasa import Module -from kasa.exceptions import KasaException from kasa.iot import IotDimmer from kasa.iot.modules.motion import Motion, Range @@ -38,32 +36,36 @@ async def test_motion_range(dev: IotDimmer, mocker: MockerFixture): motion: Motion = dev.modules[Module.IotMotion] query_helper = mocker.patch("kasa.iot.IotDimmer._query_helper") - await motion.set_range(value=123) - query_helper.assert_called_with( - "smartlife.iot.PIR", - "set_trigger_sens", - {"index": Range.Custom.value, "value": 123}, - ) + for range in Range: + await motion.set_range(range) + query_helper.assert_called_with( + "smartlife.iot.PIR", + "set_trigger_sens", + {"index": range.value}, + ) - await motion.set_range(range=Range.Custom, value=123) - query_helper.assert_called_with( - "smartlife.iot.PIR", - "set_trigger_sens", - {"index": Range.Custom.value, "value": 123}, - ) - await motion.set_range(range=Range.Far) - query_helper.assert_called_with( - "smartlife.iot.PIR", "set_trigger_sens", {"index": Range.Far.value} - ) +@dimmer_iot +async def test_motion_threshold(dev: IotDimmer, mocker: MockerFixture): + motion: Motion = dev.modules[Module.IotMotion] + query_helper = mocker.patch("kasa.iot.IotDimmer._query_helper") - with pytest.raises(KasaException, match="Refusing to set non-custom range"): - await motion.set_range(range=Range.Near, value=100) # type: ignore[call-overload] + for range in Range: + # Switch to a given range. + await motion.set_range(range) + query_helper.assert_called_with( + "smartlife.iot.PIR", + "set_trigger_sens", + {"index": range.value}, + ) - with pytest.raises( - KasaException, match="Either range or value needs to be defined" - ): - await motion.set_range() # type: ignore[call-overload] + # Assert that the range always goes to custom, regardless of current range. + await motion.set_threshold(123) + query_helper.assert_called_with( + "smartlife.iot.PIR", + "set_trigger_sens", + {"index": Range.Custom.value, "value": 123}, + ) @dimmer_iot diff --git a/tests/test_feature.py b/tests/test_feature.py index 46cdd116..932cb40f 100644 --- a/tests/test_feature.py +++ b/tests/test_feature.py @@ -140,7 +140,10 @@ async def test_feature_choice_list(dummy_feature, caplog, mocker: MockerFixture) mock_setter.assert_called_with("first") mock_setter.reset_mock() - with pytest.raises(ValueError, match="Unexpected value for dummy_feature: invalid"): # noqa: PT012 + with pytest.raises( # noqa: PT012 + ValueError, + match="Unexpected value for dummy_feature: 'invalid' (?: - allowed: .*)?", + ): await dummy_feature.set_value("invalid") assert "Unexpected value" in caplog.text