Repository: intake/intake (python)

Issue:
Unbound local error when setting cache path with environment variables
Hi there,
I've been experiencing an error when providing intake with a local cache path using environment variables.
I'm using python 3.12.8 and intake 2.0.7

## Steps to reproduce

- set an environment variable (using bash on linux)

```bash
export INTAKE_CACHE="icache"
```
- start a python console and import intake

I would expect intake to store files in a folder called `icache` in the current working directory. However, I get this error:
>   File "/home/jroettenbacher/miniforge3/envs/update-requirements/lib/python3.12/site-packages/intake/config.py", line 162, in load_env
    self[k2] = val
               ^^^
UnboundLocalError: cannot access local variable 'val' where it is not associated with a value

As far as I can tell the problem arises as `ast.literal_eval()`, which loads the environment variables, does not like string input. Except it wants everything to be quoted even strings. So `'"icache"'` would work and would create a folder called `"icache"`, which I don't want. Linux can do that but Windows throws an error because of this.

The [load_env()](https://github.com/intake/intake/blob/69c6565e8bed77a61f7e71e697053cea9ca5fb4d/intake/config.py#L152) function does not handle any exception or error thrown by `ast.literal_eval()` and instead tries to assign a not defined variable to the config dictionary, which causes the error.

It's possible to work around this by not working with local environment variables but I think this should be something one can do. Or at least could do with version 1?

I'm open to work on this problem but would love some insight and guidance on where to start. Or if I'm using local environment variables the wrong way?

Best Johannes


Pull request:
Fix handling of environment variables of type string
When local environment variables of the form `INTAKE_*="any-string"` are present on import, the load_env() function would return an `UnboundLocalError`.
This fixes that.

Fixes #855 

We could think about not warning the user about this behaviour. What do you think?
Best Johannes

Patch:
diff --git a/intake/config.py b/intake/config.py
index 0bdbbfb..c730ee3 100644
--- a/intake/config.py
+++ b/intake/config.py
@@ -13,6 +13,7 @@ import copy
 import logging
 import os
 import posixpath
+import warnings
 from os.path import expanduser
 
 import yaml
@@ -35,6 +36,7 @@ defaults = {
     "extra_imports": [],
     "import_block_list": [],
     "reader_avoid": [],
+    "environment_conf_parse": "ignore",  # "error", "warn", "ignore"
 }
 
 
@@ -46,6 +48,14 @@ class Config(dict):
     """Intake's dict-like config system
 
     Instance ``intake.conf`` is globally used throughout the package
+
+    Attributes:
+        environment_conf_parse : str
+                                 "ignore" (default), "warn" or raise an "error"
+                                 when parsing local environment variables as strings.
+        #TODO: Add other config values
+
+
     """
 
     def __init__(self, filename=None, **kwargs):
@@ -157,8 +167,19 @@ class Config(dict):
                 k2 = k[7:].lower()
                 try:
                     val = ast.literal_eval(v)
-                except ValueError:
-                    pass
+                except (ValueError, SyntaxError) as e:
+                    if self["environment_conf_parse"] == "error":
+                        logger.info(
+                            f"Environment variable '{v}' cannot be parsed by ast.literal_eval()"
+                        )
+                        raise e
+                    elif self["environment_conf_parse"] == "warn":
+                        warnings.warn(
+                            f"Failed to parse environment variable '{k}' (value: '{v}') using ast.literal_eval()."
+                            f"\nParsing as string."
+                            f"\nError was: '{type(e).__name__}: {e}'"
+                        )
+                    val = str(v)  # make sure it is a string
                 self[k2] = val
 
 


Test patch:
diff --git a/intake/tests/test_config.py b/intake/tests/test_config.py
index 127c420..0bca171 100644
--- a/intake/tests/test_config.py
+++ b/intake/tests/test_config.py
@@ -38,3 +38,34 @@ def test_pathdirs():
         "memory://path1",
         "memory://path2",
     ]
+
+
+@pytest.mark.parametrize(
+    "conf",
+    [
+        {"environment_conf_parse": "error"},
+        {"environment_conf_parse": "warn"},
+        {"environment_conf_parse": "ignore"},
+    ],
+)
+def test_load_env(conf):
+    # test the parsing of environment variables as strings
+    os.environ["INTAKE_CACHE"] = "./tmp"  # this causes a SyntaxError
+    os.environ["INTAKE_CACHE2"] = "tmp"  # this causes a ValueError
+    with temp_conf(conf) as fn:
+        if conf["environment_conf_parse"] == "error":
+            # When raise_on_error is True, ensure the exception is raised
+            with pytest.raises(ValueError, SyntaxError):
+                Config(fn)
+        elif conf["environment_conf_parse"] == "warn":
+            # When raise_on_error is False, ensure the variable is parsed as a string
+            with pytest.warns(UserWarning, match="environment variable"):
+                config = Config(fn)
+            assert config["cache"] == "./tmp"
+            assert config["cache2"] == "tmp"
+        else:
+            with pytest.warns(None) as warnings:
+                config = Config(fn)
+            assert not warnings
+            assert config["cache"] == "./tmp"
+            assert config["cache2"] == "tmp"