Improve CLI handling of configuration file arguments (#131)

* Make -configuration option compulsory

This updates the stylist CLI to make the -configuration compulsory.
This will cause the command to fail early if the configuration is
incorrect, allowing ArgumentParser to display a meaningful error
message.

* Check configuration exists and has a valid extension

This updates the CLI to check whether the configuration file exists and
whether it has a valid python extension.  This allows ArgumentParser to
display a meaningful error if the configuration is not valid.

* Add name to list of contributors

As per the guidance, I have added my name to the list of contributors.

* Allow Configurations to be combined

Create an overload method which shallow merges the pipes and styles
attributes of a second Configuration into the current instance.  This
is intended to be used to allow per-site, per-user, and per-project
configurations to be combined into a single entity.

* Adapt configuration option and improve testing

Reviewer has suggested changing the argument handling to prevent the
user from having to specify an option because this is contrary to
spirit of making the flag voluntary.  Instead, the code checks a
variety of locations before raising an exception if the configuration
cannot be found.

This also improves temporary directory handling in tests as
recommended by the reviewer, replacing clunky custom code with
a function provide pytest.

* Fix type hinting of __configure()

The __configure() function return value in the main script had the
wrong type hinting.  This changes the hinting to match None in the
event of a configuration problem.

CONTRIBUTING.md

@@ -28,6 +28,7 @@ below:
 * Matt Shin (Met Office, UK)
 * Bilal Chughtai (Met Office, UK)
 * James Cuninngham-Smith (Met Office, UK)
+* Sam Clarke-Green (Met Office, UK)
 
 (All contributors are identifiable with email addresses in the version control
 logs or otherwise.)

source/stylist/__main__.py

@@ -13,7 +13,7 @@
 from pathlib import Path
 import sys
 from textwrap import indent
-from typing import List, Sequence
+from typing import List, Sequence, Union
 
 from stylist import StylistException
 from stylist.configuration import (Configuration,
@@ -25,6 +25,11 @@
 from stylist.style import Style
 
 
+# Paths to site-wide and per-user style files
+site_file = Path("/etc/stylist.py")
+user_file = Path.home() / ".stylist.py"
+
+
 def __parse_cli() -> argparse.Namespace:
     """
     Parse the command line for stylist arguments.
@@ -107,10 +112,30 @@ def __process(candidates: List[Path], styles: Sequence[Style]) -> List[Issue]:
     return issues
 
 
-def __configure(project_file: Path) -> Configuration:
-    configuration = load_configuration(project_file)
-    # TODO /etc/fab.ini
-    # TODO ~/.fab.ini - Path.home() / '.fab.ini'
+def __configure(project_file: Path) -> Union[Configuration, None]:
+    """
+    Load configuration styles in order of specificity
+
+    Load the global site configuration, the per-user configuration, and
+    finally the configuration option provided on the command line.
+    More specific options are allowed to override more general ones,
+    allowing a configuration to built up gradually.
+    """
+
+    candidates = [site_file, user_file, project_file]
+
+    configuration = None
+
+    for target in candidates:
+        if target is None or not target.exists():
+            continue
+
+        style = load_configuration(target)
+        if configuration is None:
+            configuration = style
+        else:
+            configuration.overload(style)
+
     return configuration
 
 
@@ -187,6 +212,11 @@ def main() -> None:
         logger.setLevel(logging.WARNING)
 
     configuration = __configure(arguments.configuration)
+    if configuration is None:
+        # No valid configuration files have been found
+        # FIXME: proper exit handling
+        raise Exception("no valid style files found")
+
     issues = perform(configuration,
                      arguments.source,
                      arguments.style,

source/stylist/configuration.py

@@ -9,6 +9,8 @@
 
 Configuration may be defined by software or read from a Windows .ini file.
 """
+from __future__ import annotations
+
 from importlib.util import module_from_spec, spec_from_file_location
 from pathlib import Path
 from typing import Dict, List, Tuple, Type
@@ -37,6 +39,10 @@ def add_pipe(self, extension: str, pipe: FilePipe):
     def add_style(self, name: str, style: Style):
         self._styles[name] = style
 
+    def overload(self, other: Configuration) -> None:
+        self._pipes = {**self._pipes, **other._pipes}
+        self._styles = {**self._styles, **other._styles}
+
     @property
     def file_pipes(self) -> Dict[str, FilePipe]:
         return self._pipes

unit-tests/__main__test.py

@@ -7,12 +7,13 @@
 """
 Tests the reporting of error conditions.
 """
-from pathlib import Path
 
-from pytest import raises
+from pathlib import Path
+from pytest import raises, fixture
 
 from stylist import StylistException
-from stylist.__main__ import perform
+from stylist.__main__ import perform, __configure
+import stylist.__main__ as maintest
 from stylist.configuration import Configuration
 from stylist.style import Style
 
@@ -40,3 +41,97 @@ def test_missing_style(tmp_path: Path):
         _ = perform(configuration, [tmp_path], ['missing'], [], verbose=False)
     assert ex.value.message \
            == 'Style "missing" is not defined by the configuration.'
+
+
+@fixture(scope="session")
+def site_config(tmp_path_factory):
+    site = tmp_path_factory.mktemp("data") / "site.py"
+    site.write_text("\n".join([
+        "from stylist.source import FilePipe, PlainText",
+        "from stylist.rule import LimitLineLength",
+        "from stylist.style import Style",
+        "txt = FilePipe(PlainText)",
+        "foo = Style(LimitLineLength(80))",
+    ]))
+
+    return site
+
+
+@fixture(scope="session")
+def user_config(tmp_path_factory):
+    user = tmp_path_factory.mktemp("data") / "user.py"
+    user.write_text("\n".join([
+        "from stylist.source import FilePipe, PlainText",
+        "from stylist.rule import TrailingWhitespace",
+        "from stylist.style import Style",
+        "txt = FilePipe(PlainText)",
+        "bar = Style(TrailingWhitespace())",
+    ]))
+
+    return user
+
+
+@fixture(scope="session")
+def project_config(tmp_path_factory):
+    project = tmp_path_factory.mktemp("data") / "project.py"
+    project.write_text("\n".join([
+        "from stylist.source import FilePipe, PlainText",
+        "from stylist.rule import LimitLineLength, TrailingWhitespace",
+        "from stylist.style import Style",
+        "txt = FilePipe(PlainText)",
+        "foo = Style(LimitLineLength(80), TrailingWhitespace)",
+    ]))
+
+    return project
+
+
+def test_no_configurations(tmp_path):
+
+    maintest.site_file = None
+    maintest.user_file = None
+
+    configuration = __configure(None)
+    assert configuration is None
+
+
+def test_site_only_configuration(site_config):
+
+    maintest.site_file = site_config
+    maintest.user_file = None
+
+    configuration = __configure(None)
+    assert configuration is not None
+    assert list(configuration.styles) == ["foo"]
+    assert len(configuration.styles["foo"].list_rules()) == 1
+
+
+def test_user_only_configuration(user_config):
+
+    maintest.site_file = None
+    maintest.user_file = user_config
+
+    configuration = __configure(None)
+    assert configuration is not None
+    assert list(configuration.styles) == ["bar"]
+    assert len(configuration.styles["bar"].list_rules()) == 1
+
+
+def test_user_and_site_configurations(site_config, user_config):
+
+    maintest.site_file = site_config
+    maintest.user_file = user_config
+
+    configuration = __configure(None)
+    assert configuration is not None
+    assert list(configuration.styles) == ["foo", "bar"]
+
+
+def test_all_configurations(site_config, user_config, project_config):
+
+    maintest.site_file = site_config
+    maintest.user_file = user_config
+
+    configuration = __configure(project_config)
+    assert configuration is not None
+    assert list(configuration.styles) == ["foo", "bar"]
+    assert len(configuration.styles["foo"].list_rules()) == 2

unit-tests/configuration_test.py

@@ -177,3 +177,57 @@ def test_regex_rule(self, tmp_path: Path):
         assert len(style.list_rules()) == 1
         assert isinstance(style.list_rules()[0], DummyRuleOne)
         assert cast(DummyRuleOne, style.list_rules()[0]).first.pattern == r'.*'
+
+    def test_config_add_pipes(self, tmp_path: Path):
+        first_file = tmp_path / 'first.py'
+        first_file.write_text(dedent("""
+            from stylist.source import FilePipe
+            from configuration_test import DummySource
+            foo = FilePipe(DummySource)
+            """))
+
+        second_file = tmp_path / 'second.py'
+        second_file.write_text(dedent("""
+            from stylist.source import FilePipe
+            from configuration_test import DummyProcOne, DummySource
+            foo = FilePipe(DummySource, DummyProcOne)
+            """))
+
+        configuration = load_configuration(first_file)
+        assert list(configuration.file_pipes.keys()) == ['foo']
+        style = configuration.file_pipes['foo']
+        assert style.parser == DummySource
+        assert len(style.preprocessors) == 0
+        assert configuration.styles == {}
+
+        configuration.overload(load_configuration(second_file))
+        assert list(configuration.file_pipes.keys()) == ['foo']
+        style = configuration.file_pipes['foo']
+        assert style.parser == DummySource
+        assert len(style.preprocessors) == 1
+        assert configuration.styles == {}
+
+    def test_config_add_styles(self, tmp_path: Path):
+        first_file = tmp_path / 'first.py'
+        first_file.write_text(dedent("""
+            from stylist.style import Style
+            from configuration_test import DummyRuleZero
+            foo = Style(DummyRuleZero())
+            """))
+
+        second_file = tmp_path / 'second.py'
+        second_file.write_text(dedent("""
+            from stylist.style import Style
+            from configuration_test import DummyRuleOne
+            foo = Style(DummyRuleOne(1))
+            """))
+
+        configuration = load_configuration(first_file)
+        assert list(configuration.styles.keys()) == ['foo']
+        style = configuration.styles['foo']
+        assert isinstance(style.list_rules()[0], DummyRuleZero)
+
+        configuration.overload(load_configuration(second_file))
+        assert list(configuration.styles.keys()) == ['foo']
+        style = configuration.styles['foo']
+        assert isinstance(style.list_rules()[0], DummyRuleOne)