From 52818bdb891387e4b9eaa30ec0540e532c7cb78e Mon Sep 17 00:00:00 2001 From: Paulus Schoutsen Date: Thu, 19 Dec 2019 14:00:22 +0100 Subject: [PATCH] Make Hassfest stricter pt 2 (#30068) * Make Hassfest stricter * Fix if-condition * Small cleanup --- .../components/modbus/binary_sensor.py | 3 +- homeassistant/components/modbus/switch.py | 2 +- homeassistant/components/mqtt/climate.py | 10 +- homeassistant/components/tuya/climate.py | 6 +- homeassistant/components/zamg/sensor.py | 21 +-- script/hassfest/dependencies.py | 151 ++++++++++++------ tests/hassfest/test_dependencies.py | 17 +- 7 files changed, 130 insertions(+), 80 deletions(-) diff --git a/homeassistant/components/modbus/binary_sensor.py b/homeassistant/components/modbus/binary_sensor.py index cbefd1271d0..faf5160d7e5 100644 --- a/homeassistant/components/modbus/binary_sensor.py +++ b/homeassistant/components/modbus/binary_sensor.py @@ -3,8 +3,7 @@ import logging import voluptuous as vol -from homeassistant.components.binary_sensor import BinarySensorDevice -from homeassistant.components.sensor import PLATFORM_SCHEMA +from homeassistant.components.binary_sensor import PLATFORM_SCHEMA, BinarySensorDevice from homeassistant.const import CONF_NAME, CONF_SLAVE from homeassistant.helpers import config_validation as cv diff --git a/homeassistant/components/modbus/switch.py b/homeassistant/components/modbus/switch.py index 43ef649f788..eba0c754f45 100644 --- a/homeassistant/components/modbus/switch.py +++ b/homeassistant/components/modbus/switch.py @@ -3,7 +3,7 @@ import logging import voluptuous as vol -from homeassistant.components.sensor import PLATFORM_SCHEMA +from homeassistant.components.switch import PLATFORM_SCHEMA from homeassistant.const import ( CONF_COMMAND_OFF, CONF_COMMAND_ON, diff --git a/homeassistant/components/mqtt/climate.py b/homeassistant/components/mqtt/climate.py index a51590e0b59..f9d7d8752f2 100644 --- a/homeassistant/components/mqtt/climate.py +++ b/homeassistant/components/mqtt/climate.py @@ -14,6 +14,10 @@ from homeassistant.components.climate.const import ( ATTR_TARGET_TEMP_LOW, DEFAULT_MAX_TEMP, DEFAULT_MIN_TEMP, + FAN_AUTO, + FAN_HIGH, + FAN_LOW, + FAN_MEDIUM, HVAC_MODE_AUTO, HVAC_MODE_COOL, HVAC_MODE_DRY, @@ -29,7 +33,6 @@ from homeassistant.components.climate.const import ( SUPPORT_TARGET_TEMPERATURE, SUPPORT_TARGET_TEMPERATURE_RANGE, ) -from homeassistant.components.fan import SPEED_HIGH, SPEED_LOW, SPEED_MEDIUM from homeassistant.const import ( ATTR_TEMPERATURE, CONF_DEVICE, @@ -165,8 +168,7 @@ PLATFORM_SCHEMA = ( vol.Optional(CONF_DEVICE): mqtt.MQTT_ENTITY_DEVICE_INFO_SCHEMA, vol.Optional(CONF_FAN_MODE_COMMAND_TOPIC): mqtt.valid_publish_topic, vol.Optional( - CONF_FAN_MODE_LIST, - default=[HVAC_MODE_AUTO, SPEED_LOW, SPEED_MEDIUM, SPEED_HIGH], + CONF_FAN_MODE_LIST, default=[FAN_AUTO, FAN_LOW, FAN_MEDIUM, FAN_HIGH], ): cv.ensure_list, vol.Optional(CONF_FAN_MODE_STATE_TEMPLATE): cv.template, vol.Optional(CONF_FAN_MODE_STATE_TOPIC): mqtt.valid_subscribe_topic, @@ -339,7 +341,7 @@ class MqttClimate( self._target_temp_high = config[CONF_TEMP_INITIAL] if self._topic[CONF_FAN_MODE_STATE_TOPIC] is None: - self._current_fan_mode = SPEED_LOW + self._current_fan_mode = FAN_LOW if self._topic[CONF_SWING_MODE_STATE_TOPIC] is None: self._current_swing_mode = HVAC_MODE_OFF if self._topic[CONF_MODE_STATE_TOPIC] is None: diff --git a/homeassistant/components/tuya/climate.py b/homeassistant/components/tuya/climate.py index 6450920b806..eb0ef5eca2f 100644 --- a/homeassistant/components/tuya/climate.py +++ b/homeassistant/components/tuya/climate.py @@ -1,6 +1,9 @@ """Support for the Tuya climate devices.""" from homeassistant.components.climate import ENTITY_ID_FORMAT, ClimateDevice from homeassistant.components.climate.const import ( + FAN_HIGH, + FAN_LOW, + FAN_MEDIUM, HVAC_MODE_AUTO, HVAC_MODE_COOL, HVAC_MODE_FAN_ONLY, @@ -9,7 +12,6 @@ from homeassistant.components.climate.const import ( SUPPORT_FAN_MODE, SUPPORT_TARGET_TEMPERATURE, ) -from homeassistant.components.fan import SPEED_HIGH, SPEED_LOW, SPEED_MEDIUM from homeassistant.const import ( ATTR_TEMPERATURE, PRECISION_WHOLE, @@ -30,7 +32,7 @@ HA_STATE_TO_TUYA = { TUYA_STATE_TO_HA = {value: key for key, value in HA_STATE_TO_TUYA.items()} -FAN_MODES = {SPEED_LOW, SPEED_MEDIUM, SPEED_HIGH} +FAN_MODES = {FAN_LOW, FAN_MEDIUM, FAN_HIGH} def setup_platform(hass, config, add_entities, discovery_info=None): diff --git a/homeassistant/components/zamg/sensor.py b/homeassistant/components/zamg/sensor.py index c984b13ef4b..44c216eb1be 100644 --- a/homeassistant/components/zamg/sensor.py +++ b/homeassistant/components/zamg/sensor.py @@ -11,15 +11,8 @@ import pytz import requests import voluptuous as vol -from homeassistant.components.weather import ( - ATTR_WEATHER_ATTRIBUTION, - ATTR_WEATHER_HUMIDITY, - ATTR_WEATHER_PRESSURE, - ATTR_WEATHER_TEMPERATURE, - ATTR_WEATHER_WIND_BEARING, - ATTR_WEATHER_WIND_SPEED, -) from homeassistant.const import ( + ATTR_ATTRIBUTION, CONF_LATITUDE, CONF_LONGITUDE, CONF_MONITORED_CONDITIONS, @@ -43,15 +36,15 @@ DEFAULT_NAME = "zamg" MIN_TIME_BETWEEN_UPDATES = timedelta(minutes=10) SENSOR_TYPES = { - ATTR_WEATHER_PRESSURE: ("Pressure", "hPa", "LDstat hPa", float), + "pressure": ("Pressure", "hPa", "LDstat hPa", float), "pressure_sealevel": ("Pressure at Sea Level", "hPa", "LDred hPa", float), - ATTR_WEATHER_HUMIDITY: ("Humidity", "%", "RF %", int), - ATTR_WEATHER_WIND_SPEED: ("Wind Speed", "km/h", "WG km/h", float), - ATTR_WEATHER_WIND_BEARING: ("Wind Bearing", "°", "WR °", int), + "humidity": ("Humidity", "%", "RF %", int), + "wind_speed": ("Wind Speed", "km/h", "WG km/h", float), + "wind_bearing": ("Wind Bearing", "°", "WR °", int), "wind_max_speed": ("Top Wind Speed", "km/h", "WSG km/h", float), "wind_max_bearing": ("Top Wind Bearing", "°", "WSR °", int), "sun_last_hour": ("Sun Last Hour", "%", "SO %", int), - ATTR_WEATHER_TEMPERATURE: ("Temperature", "°C", "T °C", float), + "temperature": ("Temperature", "°C", "T °C", float), "precipitation": ("Precipitation", "l/m²", "N l/m²", float), "dewpoint": ("Dew Point", "°C", "TP °C", float), # The following probably not useful for general consumption, @@ -140,7 +133,7 @@ class ZamgSensor(Entity): def device_state_attributes(self): """Return the state attributes.""" return { - ATTR_WEATHER_ATTRIBUTION: ATTRIBUTION, + ATTR_ATTRIBUTION: ATTRIBUTION, ATTR_STATION: self.probe.get_data("station_name"), ATTR_UPDATED: self.probe.last_update.isoformat(), } diff --git a/script/hassfest/dependencies.py b/script/hassfest/dependencies.py index c67779d3c33..cb58de3af76 100644 --- a/script/hassfest/dependencies.py +++ b/script/hassfest/dependencies.py @@ -1,5 +1,6 @@ """Validate dependencies.""" import ast +from pathlib import Path from typing import Dict, Set from homeassistant.requirements import DISCOVERY_INTEGRATIONS @@ -13,21 +14,25 @@ class ImportCollector(ast.NodeVisitor): def __init__(self, integration: Integration): """Initialize the import collector.""" self.integration = integration - self.referenced: Set[str] = set() + self.referenced: Dict[Path, Set[str]] = {} - def maybe_add_reference(self, reference_domain: str): + # Current file or dir we're inspecting + self._cur_fil_dir = None + + def collect(self) -> None: + """Collect imports from a source file.""" + for fil in self.integration.path.glob("**/*.py"): + if not fil.is_file(): + continue + + self._cur_fil_dir = fil.relative_to(self.integration.path) + self.referenced[self._cur_fil_dir] = set() + self.visit(ast.parse(fil.read_text())) + self._cur_fil_dir = None + + def _add_reference(self, reference_domain: str): """Add a reference.""" - if ( - # If it's importing something from itself - reference_domain == self.integration.path.name - # Platform file - or (self.integration.path / f"{reference_domain}.py").exists() - # Platform dir - or (self.integration.path / reference_domain).exists() - ): - return - - self.referenced.add(reference_domain) + self.referenced[self._cur_fil_dir].add(reference_domain) def visit_ImportFrom(self, node): """Visit ImportFrom node.""" @@ -37,19 +42,19 @@ class ImportCollector(ast.NodeVisitor): if node.module.startswith("homeassistant.components."): # from homeassistant.components.alexa.smart_home import EVENT_ALEXA_SMART_HOME # from homeassistant.components.logbook import bla - self.maybe_add_reference(node.module.split(".")[2]) + self._add_reference(node.module.split(".")[2]) elif node.module == "homeassistant.components": # from homeassistant.components import sun for name_node in node.names: - self.maybe_add_reference(name_node.name) + self._add_reference(name_node.name) def visit_Import(self, node): """Visit Import node.""" # import homeassistant.components.hue as hue for name_node in node.names: if name_node.name.startswith("homeassistant.components."): - self.maybe_add_reference(name_node.name.split(".")[2]) + self._add_reference(name_node.name.split(".")[2]) def visit_Attribute(self, node): """Visit Attribute node.""" @@ -77,7 +82,7 @@ class ImportCollector(ast.NodeVisitor): ) ) ): - self.maybe_add_reference(node.attr) + self._add_reference(node.attr) else: # Have it visit other kids self.generic_visit(node) @@ -133,45 +138,93 @@ IGNORE_VIOLATIONS = [ ] -def validate_dependencies(integration: Integration): - """Validate all dependencies.""" - # Find usage of hass.components - collector = ImportCollector(integration) - - for fil in integration.path.glob("**/*.py"): - if not fil.is_file(): - continue - - collector.visit(ast.parse(fil.read_text())) - - referenced = ( - collector.referenced - - ALLOWED_USED_COMPONENTS - - set(integration.manifest["dependencies"]) - - set(integration.manifest.get("after_dependencies", [])) +def calc_allowed_references(integration: Integration) -> Set[str]: + """Return a set of allowed references.""" + allowed_references = ( + ALLOWED_USED_COMPONENTS + | set(integration.manifest["dependencies"]) + | set(integration.manifest.get("after_dependencies", [])) ) # Discovery requirements are ok if referenced in manifest for check_domain, to_check in DISCOVERY_INTEGRATIONS.items(): - if check_domain in referenced and any( - check in integration.manifest for check in to_check - ): - referenced.remove(check_domain) + if any(check in integration.manifest for check in to_check): + allowed_references.add(check_domain) - if referenced: - for domain in sorted(referenced): - if ( - integration.domain in IGNORE_VIOLATIONS - or (integration.domain, domain) in IGNORE_VIOLATIONS + return allowed_references + + +def find_non_referenced_integrations( + integrations: Dict[str, Integration], + integration: Integration, + references: Dict[Path, Set[str]], +): + """Find intergrations that are not allowed to be referenced.""" + allowed_references = calc_allowed_references(integration) + referenced = set() + for path, refs in references.items(): + if len(path.parts) == 1: + # climate.py is stored as climate + cur_fil_dir = path.stem + else: + # climate/__init__.py is stored as climate + cur_fil_dir = path.parts[0] + + is_platform_other_integration = cur_fil_dir in integrations + + for ref in refs: + # We are always allowed to import from ourselves + if ref == integration.domain: + continue + + # These references are approved based on the manifest + if ref in allowed_references: + continue + + # Some violations are whitelisted + if (integration.domain, ref) in IGNORE_VIOLATIONS: + continue + + # If it's a platform for another integration, the other integration is ok + if is_platform_other_integration and cur_fil_dir == ref: + continue + + # These have a platform specified in this integration + if not is_platform_other_integration and ( + (integration.path / f"{ref}.py").is_file() + # Platform dir + or (integration.path / ref).is_dir() ): continue - integration.add_error( - "dependencies", - "Using component {} but it's not in 'dependencies' or 'after_dependencies'".format( - domain - ), - ) + referenced.add(ref) + + return referenced + + +def validate_dependencies( + integrations: Dict[str, Integration], integration: Integration +): + """Validate all dependencies.""" + # Some integrations are allowed to have violations. + if integration.domain in IGNORE_VIOLATIONS: + return + + # Find usage of hass.components + collector = ImportCollector(integration) + collector.collect() + + for domain in sorted( + find_non_referenced_integrations( + integrations, integration, collector.referenced + ) + ): + integration.add_error( + "dependencies", + "Using component {} but it's not in 'dependencies' or 'after_dependencies'".format( + domain + ), + ) def validate(integrations: Dict[str, Integration], config): @@ -181,7 +234,7 @@ def validate(integrations: Dict[str, Integration], config): if not integration.manifest: continue - validate_dependencies(integration) + validate_dependencies(integrations, integration) # check that all referenced dependencies exist for dep in integration.manifest["dependencies"]: diff --git a/tests/hassfest/test_dependencies.py b/tests/hassfest/test_dependencies.py index 0f07c45317e..b9690107619 100644 --- a/tests/hassfest/test_dependencies.py +++ b/tests/hassfest/test_dependencies.py @@ -9,7 +9,8 @@ from script.hassfest.dependencies import ImportCollector def mock_collector(): """Fixture with import collector that adds all referenced nodes.""" collector = ImportCollector(None) - collector.maybe_add_reference = collector.referenced.add + collector.unfiltered_referenced = set() + collector._add_reference = collector.unfiltered_referenced.add return collector @@ -22,7 +23,7 @@ from homeassistant.components import child_import """ ) ) - assert mock_collector.referenced == {"child_import"} + assert mock_collector.unfiltered_referenced == {"child_import"} def test_subimport(mock_collector): @@ -34,7 +35,7 @@ from homeassistant.components.subimport.smart_home import EVENT_ALEXA_SMART_HOME """ ) ) - assert mock_collector.referenced == {"subimport"} + assert mock_collector.unfiltered_referenced == {"subimport"} def test_child_import_field(mock_collector): @@ -46,7 +47,7 @@ from homeassistant.components.child_import_field import bla """ ) ) - assert mock_collector.referenced == {"child_import_field"} + assert mock_collector.unfiltered_referenced == {"child_import_field"} def test_renamed_absolute(mock_collector): @@ -58,7 +59,7 @@ import homeassistant.components.renamed_absolute as hue """ ) ) - assert mock_collector.referenced == {"renamed_absolute"} + assert mock_collector.unfiltered_referenced == {"renamed_absolute"} def test_hass_components_var(mock_collector): @@ -71,7 +72,7 @@ def bla(hass): """ ) ) - assert mock_collector.referenced == {"hass_components_var"} + assert mock_collector.unfiltered_referenced == {"hass_components_var"} def test_hass_components_class(mock_collector): @@ -85,7 +86,7 @@ class Hello: """ ) ) - assert mock_collector.referenced == {"hass_components_class"} + assert mock_collector.unfiltered_referenced == {"hass_components_class"} def test_all_imports(mock_collector): @@ -110,7 +111,7 @@ class Hello: """ ) ) - assert mock_collector.referenced == { + assert mock_collector.unfiltered_referenced == { "child_import", "subimport", "child_import_field",