From 2326a2941e086f5b0dc4f4af2fde9b7f07d2e947 Mon Sep 17 00:00:00 2001 From: Paulus Schoutsen Date: Thu, 16 Apr 2020 18:00:30 -0700 Subject: [PATCH] Improve error message when people have not moved config flow title yet (#34321) --- script/hassfest/__main__.py | 28 ++++++-- script/hassfest/model.py | 5 ++ script/hassfest/translations.py | 124 ++++++++++++++++++++++---------- 3 files changed, 114 insertions(+), 43 deletions(-) diff --git a/script/hassfest/__main__.py b/script/hassfest/__main__.py index 00fd30b5278c..aec1fd34b155 100644 --- a/script/hassfest/__main__.py +++ b/script/hassfest/__main__.py @@ -125,18 +125,22 @@ def main(): general_errors = config.errors invalid_itg = [itg for itg in integrations.values() if itg.errors] + warnings_itg = [itg for itg in integrations.values() if itg.warnings] + print() print("Integrations:", len(integrations)) print("Invalid integrations:", len(invalid_itg)) + print() if not invalid_itg and not general_errors: + print_integrations_status(config, warnings_itg, show_fixable_errors=False) + if config.action == "generate": for plugin in plugins: if hasattr(plugin, "generate"): plugin.generate(integrations, config) return 0 - print() if config.action == "generate": print("Found errors. Generating files canceled.") print() @@ -147,15 +151,25 @@ def main(): print("*", error) print() - for integration in sorted(invalid_itg, key=lambda itg: itg.domain): - extra = f" - {integration.path}" if config.specific_integrations else "" - print(f"Integration {integration.domain}{extra}:") - for error in integration.errors: - print("*", error) - print() + invalid_itg.extend(itg for itg in warnings_itg if itg not in invalid_itg) + + print_integrations_status(config, invalid_itg, show_fixable_errors=False) return 1 +def print_integrations_status(config, integrations, *, show_fixable_errors=True): + """Print integration status.""" + for integration in sorted(integrations, key=lambda itg: itg.domain): + extra = f" - {integration.path}" if config.specific_integrations else "" + print(f"Integration {integration.domain}{extra}:") + for error in integration.errors: + if show_fixable_errors or not error.fixable: + print("*", error) + for warning in integration.warnings: + print("*", "[WARNING]", warning) + print() + + if __name__ == "__main__": sys.exit(main()) diff --git a/script/hassfest/model.py b/script/hassfest/model.py index a03bc3ebd00a..e90a08f69fea 100644 --- a/script/hassfest/model.py +++ b/script/hassfest/model.py @@ -66,6 +66,7 @@ class Integration: path = attr.ib(type=pathlib.Path) manifest = attr.ib(type=dict, default=None) errors = attr.ib(type=List[Error], factory=list) + warnings = attr.ib(type=List[Error], factory=list) @property def domain(self) -> str: @@ -86,6 +87,10 @@ class Integration: """Add an error.""" self.errors.append(Error(*args, **kwargs)) + def add_warning(self, *args, **kwargs): + """Add an warning.""" + self.warnings.append(Error(*args, **kwargs)) + def load_manifest(self) -> None: """Load manifest.""" manifest_path = self.path / "manifest.json" diff --git a/script/hassfest/translations.py b/script/hassfest/translations.py index f55e793ad923..1287e82885f4 100644 --- a/script/hassfest/translations.py +++ b/script/hassfest/translations.py @@ -1,17 +1,48 @@ """Validate integration translation files.""" +from functools import partial import json +import logging from typing import Dict import voluptuous as vol from voluptuous.humanize import humanize_error -from .model import Integration +from .model import Config, Integration + +_LOGGER = logging.getLogger(__name__) + +UNDEFINED = 0 +REQUIRED = 1 +REMOVED = 2 + +REMOVED_TITLE_MSG = ( + "config.title key has been moved out of config and into the root of strings.json. " + "Starting Home Assistant 0.109 you only need to define this key in the root " + "if the title needs to be different than the name of your integration in the " + "manifest." +) -def data_entry_schema(*, require_title: bool, require_step_title: bool): +def removed_title_validator(config, integration, value): + """Mark removed title.""" + if not config.specific_integrations: + raise vol.Invalid(REMOVED_TITLE_MSG) + + # Don't mark it as an error yet for custom components to allow backwards compat. + integration.add_warning("translations", REMOVED_TITLE_MSG) + return value + + +def gen_data_entry_schema( + *, + config: Config, + integration: Integration, + flow_title: int, + require_step_title: bool, +): """Generate a data entry schema.""" step_title_class = vol.Required if require_step_title else vol.Optional - data_entry_schema = { + schema = { vol.Optional("flow_title"): str, vol.Required("step"): { str: { @@ -24,43 +55,64 @@ def data_entry_schema(*, require_title: bool, require_step_title: bool): vol.Optional("abort"): {str: str}, vol.Optional("create_entry"): {str: str}, } - if require_title: - data_entry_schema[vol.Required("title")] = str + if flow_title == REQUIRED: + schema[vol.Required("title")] = str + elif flow_title == REMOVED: + schema[vol.Optional("title", msg=REMOVED_TITLE_MSG)] = partial( + removed_title_validator, config, integration + ) - return data_entry_schema + return schema -STRINGS_SCHEMA = vol.Schema( - { - vol.Optional("title"): str, - vol.Optional("config"): data_entry_schema( - require_title=False, require_step_title=True - ), - vol.Optional("options"): data_entry_schema( - require_title=False, require_step_title=False - ), - vol.Optional("device_automation"): { - vol.Optional("action_type"): {str: str}, - vol.Optional("condition_type"): {str: str}, - vol.Optional("trigger_type"): {str: str}, - vol.Optional("trigger_subtype"): {str: str}, - }, - vol.Optional("state"): {str: str}, - } -) - -AUTH_SCHEMA = vol.Schema( - { - vol.Optional("mfa_setup"): { - str: data_entry_schema(require_title=True, require_step_title=True) +def gen_strings_schema(config: Config, integration: Integration): + """Generate a strings schema.""" + return vol.Schema( + { + vol.Optional("title"): str, + vol.Optional("config"): gen_data_entry_schema( + config=config, + integration=integration, + flow_title=REMOVED, + require_step_title=True, + ), + vol.Optional("options"): gen_data_entry_schema( + config=config, + integration=integration, + flow_title=UNDEFINED, + require_step_title=False, + ), + vol.Optional("device_automation"): { + vol.Optional("action_type"): {str: str}, + vol.Optional("condition_type"): {str: str}, + vol.Optional("trigger_type"): {str: str}, + vol.Optional("trigger_subtype"): {str: str}, + }, + vol.Optional("state"): {str: str}, } - } -) + ) + + +def gen_auth_schema(config: Config, integration: Integration): + """Generate auth schema.""" + return vol.Schema( + { + vol.Optional("mfa_setup"): { + str: gen_data_entry_schema( + config=config, + integration=integration, + flow_title=REQUIRED, + require_step_title=True, + ) + } + } + ) + ONBOARDING_SCHEMA = vol.Schema({vol.Required("area"): {str: str}}) -def validate_translation_file(integration: Integration): +def validate_translation_file(config: Config, integration: Integration): """Validate translation files for integration.""" strings_file = integration.path / "strings.json" @@ -70,11 +122,11 @@ def validate_translation_file(integration: Integration): strings = json.loads(strings_file.read_text()) if integration.domain == "auth": - schema = AUTH_SCHEMA + schema = gen_auth_schema(config, integration) elif integration.domain == "onboarding": schema = ONBOARDING_SCHEMA else: - schema = STRINGS_SCHEMA + schema = gen_strings_schema(config, integration) try: schema(strings) @@ -84,7 +136,7 @@ def validate_translation_file(integration: Integration): ) -def validate(integrations: Dict[str, Integration], config): +def validate(integrations: Dict[str, Integration], config: Config): """Handle JSON files inside integrations.""" for integration in integrations.values(): - validate_translation_file(integration) + validate_translation_file(config, integration)