Repository: peopledoc/septentrion

Issue:
UnboundLocalError: local variable 'file_contents' referenced before assignment
If interpreter goes there: https://github.com/peopledoc/septentrion/blob/master/septentrion/cli.py#L42
On the line https://github.com/peopledoc/septentrion/blob/master/septentrion/cli.py#L49 it raises `UnboundLocalError`, but not the `exceptions.NoSeptentrionSection`. 


Discussion:
ewjoachim: Ping @alexdashkov : I believe you were interested in this bug too.
codecov[bot]: # [Codecov](https://codecov.io/gh/peopledoc/septentrion/pull/67?src=pr&el=h1) Report
> Merging [#67](https://codecov.io/gh/peopledoc/septentrion/pull/67?src=pr&el=desc) into [master](https://codecov.io/gh/peopledoc/septentrion/commit/194a807e9e276c3bfa6e9b439ecbb1cadf435c4e&el=desc) will **increase** coverage by `2.18%`.
> The diff coverage is `89.28%`.

[![Impacted file tree graph](https://codecov.io/gh/peopledoc/septentrion/pull/67/graphs/tree.svg?width=650&height=150&src=pr&token=z90bSnoN0U)](https://codecov.io/gh/peopledoc/septentrion/pull/67?src=pr&el=tree)

```diff
@@            Coverage Diff             @@
##           master      #67      +/-   ##
==========================================
+ Coverage   72.10%   74.28%   +2.18%     
==========================================
  Files          15       15              
  Lines         656      665       +9     
  Branches       87       91       +4     
==========================================
+ Hits          473      494      +21     
+ Misses        171      158      -13     
- Partials       12       13       +1     
```

| Flag | Coverage Δ | |
|---|---|---|
| #integration | `47.81% <28.57%> (-0.05%)` | :arrow_down: |
| #unit | `63.60% <89.28%> (+2.32%)` | :arrow_up: |

| [Impacted Files](https://codecov.io/gh/peopledoc/septentrion/pull/67?src=pr&el=tree) | Coverage Δ | |
|---|---|---|
| [septentrion/cli.py](https://codecov.io/gh/peopledoc/septentrion/pull/67/diff?src=pr&el=tree#diff-c2VwdGVudHJpb24vY2xpLnB5) | `0.00% <0.00%> (ø)` | |
| [septentrion/configuration.py](https://codecov.io/gh/peopledoc/septentrion/pull/67/diff?src=pr&el=tree#diff-c2VwdGVudHJpb24vY29uZmlndXJhdGlvbi5weQ==) | `83.14% <92.59%> (+2.84%)` | :arrow_up: |

------

[Continue to review full report at Codecov](https://codecov.io/gh/peopledoc/septentrion/pull/67?src=pr&el=continue).
> **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta)
> `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
> Powered by [Codecov](https://codecov.io/gh/peopledoc/septentrion/pull/67?src=pr&el=footer). Last update [194a807...cda3ce8](https://codecov.io/gh/peopledoc/septentrion/pull/67?src=pr&el=lastupdated). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments).


Patch:
diff --git a/septentrion/cli.py b/septentrion/cli.py
index d0cb4cc..5734d55 100644
--- a/septentrion/cli.py
+++ b/septentrion/cli.py
@@ -34,27 +34,7 @@ click.option = functools.partial(click.option, show_default=True)  # type: ignor
 
 
 def load_config(ctx: click.Context, param: click.Parameter, value: TextIO) -> None:
-    if not value:
-        try:
-            file_contents, file = configuration.read_default_configuration_files()
-        except exceptions.NoDefaultConfiguration:
-            pass
-    else:
-        file = getattr(value, "name", "stdin")
-        logger.info(f"Reading configuration from {file}")
-        file_contents = value.read()
-
-    try:
-        default = configuration.parse_configuration_file(file_contents)
-    except exceptions.NoSeptentrionSection:
-        if file in configuration.DEDICATED_CONFIGURATION_FILES:
-            click.echo(
-                f"Configuration file found at {file} but contains no septentrion "
-                "section"
-            )
-        default = {}
-
-    ctx.default_map = default
+    ctx.default_map = configuration.load_configuration_files(value)
 
 
 CONTEXT_SETTINGS = {
@@ -96,7 +76,7 @@ class CommaSeparatedMultipleString(StringParamType):
     is_eager=True,
     callback=load_config,
     help="Config file to use (env: SEPTENTRION_CONFIG_FILE)  "
-    f"[default: {' or '.join(configuration.ALL_CONFIGURATION_FILES)}]",
+    f"[default: {' or '.join(str(p) for p in configuration.ALL_CONFIGURATION_FILES)}]",
     type=click.File("rb"),
 )
 @click.version_option(__version__, "-V", "--version", prog_name="septentrion")
diff --git a/septentrion/configuration.py b/septentrion/configuration.py
index fa4b877..9d5b329 100644
--- a/septentrion/configuration.py
+++ b/septentrion/configuration.py
@@ -5,21 +5,27 @@ the settings, see cli.py (for now)
 import configparser
 import logging
 import pathlib
-from typing import Any, Dict, Tuple, Union
+from typing import Any, Dict, Optional, TextIO, Tuple, Union
 
 from septentrion import exceptions
 
 logger = logging.getLogger(__name__)
 
 # These are the configuration files only used by septentrion
+DEDICATED_CONFIGURATION_FILENAME = "septentrion.ini"
+CONFIGURATION_PATHS = [
+    pathlib.Path("./"),
+    pathlib.Path("~/.config/"),
+    pathlib.Path("/etc/"),
+]
 DEDICATED_CONFIGURATION_FILES = [
-    "./septentrion.ini",
-    "~/.config/septentrion.ini",
-    "/etc/septentrion.ini",
+    folder.expanduser().resolve() / DEDICATED_CONFIGURATION_FILENAME
+    for folder in CONFIGURATION_PATHS
 ]
+print(DEDICATED_CONFIGURATION_FILES)
 # These are the files that can contain septentrion configuration, but
 # it's also ok if they exist and they don't configure septentrion.
-COMMON_CONFIGURATION_FILES = ["./setup.cfg"]
+COMMON_CONFIGURATION_FILES = [pathlib.Path("./setup.cfg")]
 
 ALL_CONFIGURATION_FILES = DEDICATED_CONFIGURATION_FILES + COMMON_CONFIGURATION_FILES
 
@@ -44,7 +50,7 @@ DEFAULTS = {
 }
 
 
-def read_default_configuration_files() -> Tuple[str, str]:
+def read_default_configuration_files() -> Tuple[str, pathlib.Path]:
     for file in ALL_CONFIGURATION_FILES:
         try:
             return read_configuration_file(file), file
@@ -56,7 +62,7 @@ def read_default_configuration_files() -> Tuple[str, str]:
     raise exceptions.NoDefaultConfiguration
 
 
-def read_configuration_file(path: str) -> str:
+def read_configuration_file(path: pathlib.Path) -> str:
     with open(path, "r") as handler:
         logger.info(f"Reading configuration from {path}")
         return handler.read()
@@ -142,3 +148,39 @@ class Settings:
     def update(self, values: Dict) -> None:
         for key, value in values.items():
             self.set(key, value)
+
+
+def load_configuration_files(value: Optional[TextIO]) -> Dict[str, Any]:
+    """
+    Load configuration from default source files.
+    """
+    expected_section = True
+    file: Union[pathlib.Path, str]
+
+    if not value:
+        try:
+            file_contents, file = read_default_configuration_files()
+        except exceptions.NoDefaultConfiguration:
+            return {}
+        if file not in DEDICATED_CONFIGURATION_FILES:
+            expected_section = False
+    else:
+        if hasattr(value, "name"):
+            file = pathlib.Path(value.name)
+        else:
+            file = "stdin"
+        logger.info(f"Reading configuration from {file}")
+
+        file_contents = value.read()
+
+    try:
+        return parse_configuration_file(file_contents)
+    except exceptions.NoSeptentrionSection:
+        if expected_section:
+            logger.warning(
+                f"Configuration file found at {str(file)} but contains no "
+                "septentrion section"
+            )
+
+    # No configuration found in files ; use stock default values
+    return {}


Test patch:
diff --git a/tests/acceptance/test_cli.py b/tests/acceptance/test_cli.py
index 48314f2..4701190 100644
--- a/tests/acceptance/test_cli.py
+++ b/tests/acceptance/test_cli.py
@@ -8,8 +8,18 @@ def test_version(cli_runner):
     )
 
 
-def test_current_database_state(cli_runner, db):
+def test_no_configuration_file(cli_runner, temporary_directory, mocker):
+    mocker.patch("septentrion.core.describe_migration_plan")
+    mocker.patch("septentrion.db.create_table")
+    result = cli_runner.invoke(
+        __main__.main,
+        ["--target-version=0.0.0", "show-migrations"],
+        catch_exceptions=False,
+    )
+    assert result.exit_code == 0, (result.output,)
+
 
+def test_current_database_state(cli_runner, db):
     result = cli_runner.invoke(
         __main__.main,
         [
diff --git a/tests/conftest.py b/tests/conftest.py
index fb9ec14..6db5deb 100644
--- a/tests/conftest.py
+++ b/tests/conftest.py
@@ -33,3 +33,9 @@ def fake_db(mocker):
     patch = mocker.patch("septentrion.db.execute")
     patch.return_value.__enter__ = fake_execute
     yield fake_execute
+
+
+@pytest.fixture()
+def temporary_directory(tmpdir):
+    with tmpdir.as_cwd():
+        yield
diff --git a/tests/unit/test_configuration.py b/tests/unit/test_configuration.py
index 4de3d49..03a3b86 100644
--- a/tests/unit/test_configuration.py
+++ b/tests/unit/test_configuration.py
@@ -1,3 +1,6 @@
+import pathlib
+from io import StringIO
+
 import pytest
 
 from septentrion import configuration, exceptions
@@ -53,3 +56,65 @@ def test_get_config_files_settings():
         "before_schema_file": ["before.sql", "another_before.sql"],
         "after_schema_file": ["after.sql", "another_after.sql"],
     }
+
+
+def test_load_configuration_files_no_file(mocker):
+    # There's no available file
+    mocker.patch(
+        "septentrion.configuration.read_default_configuration_files",
+        side_effect=exceptions.NoDefaultConfiguration,
+    )
+    s = configuration.load_configuration_files(value=None)
+    assert s == {}
+
+
+@pytest.mark.parametrize(
+    "conf, expected, has_warning",
+    [
+        ("", {}, True),
+        ("[stuff]", {}, True),
+        ("[septentrion]\na = b", {"a": "b"}, False),
+    ],
+)
+def test_load_configuration_files_value(caplog, conf, expected, has_warning):
+    value = StringIO(conf)
+    value.seek(0)
+    # Reminder: an empty StringIO is not considered as Falsy.
+    s = configuration.load_configuration_files(value=value)
+    assert s == expected
+
+    warnings = [r for r in caplog.records if r.levelname == "WARNING"]
+    assert bool(warnings) is has_warning
+    assert all(
+        ["at stdin but contains no septentrion section" in r.message for r in warnings]
+    )
+
+
+@pytest.mark.parametrize(
+    "conf, filename, expected, has_warning",
+    [
+        ("", "setup.cfg", {}, False),
+        ("[stuff]", "setup.cfg", {}, False),
+        ("[septentrion]\na = b", "setup.cfg", {"a": "b"}, False),
+        ("", "septentrion.ini", {}, True),
+        ("[stuff]", "septentrion.ini", {}, True),
+        ("[septentrion]\na = b", "septentrion.ini", {"a": "b"}, False),
+    ],
+)
+def test_load_configuration_files_default_files(
+    mocker, caplog, conf, filename, expected, has_warning
+):
+    mocker.patch(
+        "septentrion.configuration.read_default_configuration_files",
+        return_value=(conf, pathlib.Path(filename).resolve()),
+    )
+    s = configuration.load_configuration_files(value=None)
+    assert s == expected
+    warnings = [record for record in caplog.records if record.levelname == "WARNING"]
+    assert bool(warnings) is has_warning
+    assert all(
+        [
+            f"/{filename} but contains no septentrion section" in r.message
+            for r in warnings
+        ]
+    )