From 64b8717a1bb01c8cf5c913b165fd6b4f3f6c1312 Mon Sep 17 00:00:00 2001
From: Andras Schmelczer <andras@schmelczer.dev>
Date: Fri, 1 Jul 2022 22:42:31 +0200
Subject: [PATCH] Add default values for conf

---
 .../utilities/config_file/config_file.py      | 36 ++++++++++---------
 tests/utilities/data/env-bad.conf             |  2 ++
 tests/utilities/data/env.conf                 |  3 ++
 tests/utilities/test_config_file.py           | 22 +++++++-----
 4 files changed, 39 insertions(+), 24 deletions(-)
 create mode 100644 tests/utilities/data/env-bad.conf

diff --git a/src/great_ai/utilities/config_file/config_file.py b/src/great_ai/utilities/config_file/config_file.py
index c6be097..079069b 100644
--- a/src/great_ai/utilities/config_file/config_file.py
+++ b/src/great_ai/utilities/config_file/config_file.py
@@ -12,9 +12,7 @@ logger = get_logger("ConfigFile")
 
 
 class ConfigFile:
-    def __init__(
-        self, path: Union[Path, str], ignore_missing_environment_variables: bool = False
-    ) -> None:
+    def __init__(self, path: Union[Path, str]) -> None:
         if not isinstance(path, Path):
             path = Path(path)
 
@@ -22,9 +20,6 @@ class ConfigFile:
             raise FileNotFoundError(path.absolute())
 
         self._path = path
-        self._ignore_missing_environment_variables = (
-            ignore_missing_environment_variables
-        )
         self._key_values: Dict[str, str] = {}
 
         self._parse()
@@ -35,11 +30,6 @@ class ConfigFile:
 
         matches = pattern.findall(lines)
         for key, *values in matches:
-            if key in self._key_values:
-                raise KeyError(
-                    f"Key `{key}` has been already defined and its value is `{self._key_values[key]}`"
-                )
-
             try:
                 value = next(v for v in values if v)
             except StopIteration:
@@ -47,15 +37,29 @@ class ConfigFile:
                     f"Cannot parse config file ({self._path.absolute()}), error at key `{key}`"
                 )
 
+            already_exists = key in self._key_values
+            if already_exists and not value.startswith(
+                f"{ENVIRONMENT_VARIABLE_KEY_PREFIX}:"
+            ):
+                raise KeyError(
+                    f"Key `{key}` has been already defined and its value is `{self._key_values[key]}`"
+                )
+
             if value.startswith(f"{ENVIRONMENT_VARIABLE_KEY_PREFIX}:"):
                 _, value = value.split(":")
                 if value not in os.environ:
                     issue = f'The value of `{key}` contains the "{ENVIRONMENT_VARIABLE_KEY_PREFIX}` prefix but `{value}` is not defined as an environment variable'
-                    if self._ignore_missing_environment_variables:
-                        logger.warning(f"{issue}, defaulting to `None`")
+                    if already_exists:
+                        logger.warning(
+                            f"{issue}, using the default value defined above (`{self._key_values[key]}`)"
+                        )
+                        continue
                     else:
-                        raise KeyError(issue)
-                value = os.environ[value]
+                        raise KeyError(
+                            f"{issue} and no default value has been provided"
+                        )
+                else:
+                    value = os.environ[value]
 
             self._key_values[key] = value
 
@@ -84,4 +88,4 @@ class ConfigFile:
         return self._key_values.items()
 
     def __repr__(self):
-        return f"{type(self).__name__}(\n  path={self._path},\n  ignore_missing_environment_variables={self._ignore_missing_environment_variables}\n) {{{self._key_values}}}"
+        return f"{type(self).__name__}(path={self._path}) {self._key_values}"
diff --git a/tests/utilities/data/env-bad.conf b/tests/utilities/data/env-bad.conf
new file mode 100644
index 0000000..60e2362
--- /dev/null
+++ b/tests/utilities/data/env-bad.conf
@@ -0,0 +1,2 @@
+
+fourth_key =  ENV:SOMETHING 
diff --git a/tests/utilities/data/env.conf b/tests/utilities/data/env.conf
index ca78f66..b644a73 100644
--- a/tests/utilities/data/env.conf
+++ b/tests/utilities/data/env.conf
@@ -3,3 +3,6 @@ first_key=test
 
 second_key="ENV:alma"
 third_key = ENV:alma 
+
+fourth_key="this is a default value"  # will fall back to this if ENV is not defined
+fourth_key =  ENV:SOMETHING 
diff --git a/tests/utilities/test_config_file.py b/tests/utilities/test_config_file.py
index c7c6b29..5b55843 100644
--- a/tests/utilities/test_config_file.py
+++ b/tests/utilities/test_config_file.py
@@ -1,3 +1,4 @@
+import os
 import unittest
 from pathlib import Path
 
@@ -62,8 +63,6 @@ multiline
         assert c.whitespace == "hardly  matters"
 
     def test_env(self) -> None:
-        import os
-
         os.environ["alma"] = "12"
         c = ConfigFile(DATA_PATH / "env.conf")
         del os.environ["alma"]
@@ -72,13 +71,20 @@ multiline
 
     def test_env_not_exists(self) -> None:
         with pytest.raises(KeyError):
-            ConfigFile(DATA_PATH / "env.conf")
+            ConfigFile(DATA_PATH / "env-bad.conf")
 
-    def test_env_not_exists_but_ignored(self) -> None:
-        with pytest.raises(KeyError):
-            ConfigFile(
-                DATA_PATH / "env.conf", ignore_missing_environment_variables=True
-            )
+    def test_env_not_exists_fallback(self) -> None:
+        os.environ["alma"] = "12"
+        c = ConfigFile(DATA_PATH / "env.conf")
+        assert c.fourth_key == "this is a default value"
+
+    def test_env_exists_ignore_fallback(self) -> None:
+        os.environ["alma"] = "12"
+        os.environ["SOMETHING"] = "hi"
+        c = ConfigFile(DATA_PATH / "env.conf")
+        del os.environ["SOMETHING"]
+
+        assert c.fourth_key == "hi"
 
     def test_duplicate_key(self) -> None:
         with pytest.raises(KeyError):