Repository: missionpinball/mpf

Issue:
Suggest config section on typos
With a broken config such as:

```
hardware_sound_system:
    default:
        label: LISY
```

Users get errors such as: `Found a "hardware_sound_system:" section in config file xxx\config\config, but that section is not valid in machine config files.`

Ideally we would recommend `Did you mean "hardware_sound_systems"?`. The config_validator which emits those warnings has all config_spec loaded and we could run a simple string similarity comparison against all of them (see: https://stackoverflow.com/questions/17388213/find-the-similarity-metric-between-two-strings) to figure the best match (or the best matches). We probably should define a similarity threshold to prevent too distinct results.

Discussion:

Patch:
diff --git a/mpf/config_players/show_player.py b/mpf/config_players/show_player.py
index 1b721778f..b7627fdc1 100644
--- a/mpf/config_players/show_player.py
+++ b/mpf/config_players/show_player.py
@@ -67,7 +67,7 @@ class ShowPlayer(DeviceConfigPlayer):
         for key in RESERVED_KEYS:
             if key in device_settings["show_tokens"]:
                 self.raise_config_error("Key {} is not allowed in show_tokens of your show_player because it is also "
-                                        "an option in show_player. Did indent that option too far?".format(key), 1)
+                                        "an option in show_player. Did you indent that option too far?".format(key), 1)
         return device_settings
 
     def handle_subscription_change(self, value, settings, priority, context):
diff --git a/mpf/core/config_processor.py b/mpf/core/config_processor.py
index a6eb8387f..b86daec9a 100755
--- a/mpf/core/config_processor.py
+++ b/mpf/core/config_processor.py
@@ -8,6 +8,7 @@ import pickle   # nosec
 import tempfile
 
 from typing import List, Tuple, Any, Optional
+from difflib import SequenceMatcher
 
 from mpf.core.file_manager import FileManager
 from mpf.core.utility_functions import Util
@@ -121,6 +122,42 @@ class ConfigProcessor:
 
         return config
 
+    @staticmethod
+    def _find_similar_key(key, config_spec, config_type):
+        """Find the most similar key in config_spec."""
+        best_similarity = 0
+        best_key = ""
+        for existing_key, config in config_spec.items():
+            if '__valid_in__' not in config or config_type not in config['__valid_in__']:
+                continue
+            similarity = SequenceMatcher(None, existing_key, key).ratio()
+            if similarity > best_similarity:
+                best_key = existing_key
+                best_similarity = similarity
+
+        return best_key
+
+    def _check_sections(self, config, config_type, filename, ignore_unknown_sections):
+        config_spec = self.config_validator.get_config_spec()
+        if not isinstance(config, dict):
+            raise ConfigFileError("Config should be a dict: {}".format(config), 1, self.log.name, filename)
+        for k in config.keys():
+            try:
+                if config_type not in config_spec[k]['__valid_in__']:
+                    raise ConfigFileError('Found a "{}:" section in config file {}, '
+                                          'but that section is not valid in {} config '
+                                          'files (only in {}).'.format(k, filename, config_type,
+                                                                       config_spec[k]['__valid_in__']),
+                                          2, self.log.name, filename)
+            except KeyError:
+                if not ignore_unknown_sections:
+                    suggestion = self._find_similar_key(k, config_spec, config_type)
+
+                    raise ConfigFileError('Found a "{}:" section in config file {}, '
+                                          'but that section name is unknown. Did you mean "{}:" '
+                                          'instead?.'.format(k, filename, suggestion), 3, self.log.name,
+                                          filename)
+
     def _load_config_file_and_return_loaded_files(
             self, filename, config_type: str,
             ignore_unknown_sections=False) -> Tuple[dict, List[str]]:   # pragma: no cover
@@ -138,20 +175,7 @@ class ConfigProcessor:
         self.log.info('Loading config: %s', filename)
 
         if config_type in ("machine", "mode"):
-            if not isinstance(config, dict):
-                raise ConfigFileError("Config should be a dict: {}".format(config), self.log.name, "ConfigProcessor")
-            for k in config.keys():
-                try:
-                    if config_type not in self.config_validator.get_config_spec()[k][
-                            '__valid_in__']:
-                        raise ValueError('Found a "{}:" section in config file {}, '
-                                         'but that section is not valid in {} config '
-                                         'files.'.format(k, filename, config_type))
-                except KeyError:
-                    if not ignore_unknown_sections:
-                        raise ValueError('Found a "{}:" section in config file {}, '
-                                         'but that section is not valid in {} config '
-                                         'files.'.format(k, filename, config_type))
+            self._check_sections(config, config_type, filename, ignore_unknown_sections)
 
         try:
             if 'config' in config:
@@ -167,43 +191,6 @@ class ConfigProcessor:
         except TypeError:
             return dict(), []
 
-    def load_config_file(self, filename, config_type: str,
-                         ignore_unknown_sections=False) -> dict:   # pragma: no cover
-        """Load a config file."""
-        # config_type is str 'machine' or 'mode', which specifies whether this
-        # file being loaded is a machine config or a mode config file
-        expected_version_str = ConfigProcessor.get_expected_version(config_type)
-        config = FileManager.load(filename, expected_version_str, True)
-
-        if not config:
-            return dict()
-
-        for k in config.keys():
-            try:
-                if config_type not in self.config_validator.get_config_spec()[k][
-                        '__valid_in__']:
-                    raise ValueError('Found a "{}:" section in config file {}, '
-                                     'but that section is not valid in {} config '
-                                     'files.'.format(k, filename, config_type))
-            except KeyError:
-                if not ignore_unknown_sections:
-                    raise ValueError('Found a "{}:" section in config file {}, '
-                                     'but that section is not valid in {} config '
-                                     'files.'.format(k, filename, config_type))
-
-        try:
-            if 'config' in config:
-                path = os.path.split(filename)[0]
-
-                for file in Util.string_to_list(config['config']):
-                    full_file = os.path.join(path, file)
-                    config = Util.dict_merge(config,
-                                             self.load_config_file(
-                                                 full_file, config_type))
-            return config
-        except TypeError:
-            return dict()
-
     @staticmethod
     def get_expected_version(config_type: str) -> str:
         """Return the expected config or show version tag, e.g. #config_version=5."""


Test patch:
diff --git a/mpf/tests/machine_files/config_processor/typo.yaml b/mpf/tests/machine_files/config_processor/typo.yaml
new file mode 100644
index 000000000..d9273ca08
--- /dev/null
+++ b/mpf/tests/machine_files/config_processor/typo.yaml
@@ -0,0 +1,7 @@
+#config_version=5
+
+light:
+  light1:
+    number: 1
+  light2:
+    number: 2
diff --git a/mpf/tests/machine_files/config_processor/working.yaml b/mpf/tests/machine_files/config_processor/working.yaml
new file mode 100644
index 000000000..a31bad088
--- /dev/null
+++ b/mpf/tests/machine_files/config_processor/working.yaml
@@ -0,0 +1,10 @@
+#config_version=5
+
+config:
+  - working_subconfig.yaml
+
+lights:
+  light1:
+    number: 1
+  light2:
+    number: 2
diff --git a/mpf/tests/machine_files/config_processor/working_subconfig.yaml b/mpf/tests/machine_files/config_processor/working_subconfig.yaml
new file mode 100644
index 000000000..1607bca51
--- /dev/null
+++ b/mpf/tests/machine_files/config_processor/working_subconfig.yaml
@@ -0,0 +1,5 @@
+#config_version=5
+
+switches:
+  switch1:
+    number: 1
diff --git a/mpf/tests/test_ConfigProcessor.py b/mpf/tests/test_ConfigProcessor.py
new file mode 100644
index 000000000..73a41293f
--- /dev/null
+++ b/mpf/tests/test_ConfigProcessor.py
@@ -0,0 +1,57 @@
+import os
+import unittest
+
+from mpf._version import log_url
+
+from mpf.core.config_processor import ConfigProcessor
+
+from mpf.core.config_validator import ConfigValidator
+
+from mpf.core.placeholder_manager import PlaceholderManager
+from mpf.exceptions.config_file_error import ConfigFileError
+
+
+class FakeMachine:
+
+    def __init__(self):
+        self.machine_config = {
+            "logging": {"console": {"placeholder_manager": "basic"}, "file": {"placeholder_manager": "basic"}}
+        }
+        self.options = {"production": False}
+        self.placeholder_manager = PlaceholderManager(self)
+
+
+class TestConfigProcessor(unittest.TestCase):
+
+    def setUp(self):
+        self.machine = FakeMachine()
+        self.config_validator = ConfigValidator(self, False, False)
+        self.config_processor = ConfigProcessor(self.config_validator)
+        self.maxDiff = None
+
+    def test_load_with_subconfig(self):
+        """Test successful load with subconfig."""
+        config_file = os.path.join(os.path.dirname(os.path.realpath(__file__)),
+                                   "machine_files/config_processor/working.yaml")
+        config = self.config_processor.load_config_files_with_cache([config_file], "machine")
+
+        self.assertEqual(
+            {'config': ['working_subconfig.yaml'], 'lights': {'light1': {'number': 1}, 'light2': {'number': 2}},
+             'switches': {'switch1': {'number': 1}}},
+            config
+        )
+
+    def test_typo(self):
+        """Test suggestion on typo."""
+        config_file = os.path.join(os.path.dirname(os.path.realpath(__file__)),
+                                   "machine_files/config_processor/typo.yaml")
+        with self.assertRaises(ConfigFileError) as e:
+            config = self.config_processor.load_config_files_with_cache([config_file], "machine")
+
+        self.assertEqual('Config File Error in ConfigProcessor: Found a "light:" section in config '
+                         'file {config_file}, '
+                         'but that section name is unknown. Did you mean "lights:" instead?. Context: '
+                         '{config_file} '
+                         'Error Code: CFE-ConfigProcessor-3 ({})'.format(log_url.format("CFE-ConfigProcessor-3"),
+                                                                         config_file=config_file),
+                         str(e.exception))