Repository: uwefladrich/scriptengine (language: python)

Issue:
Feat/yaml load
Yaml loader task, to include yaml dictionaries into the `context`. I open it as a draft, to discuss whether this feature is needed, and where it can be improved (i.e. exclude loading dictionaries with first-level lists, include a loader argument...).
Feat/yaml load
Yaml loader task, to include yaml dictionaries into the `context`. I open it as a draft, to discuss whether this feature is needed, and where it can be improved (i.e. exclude loading dictionaries with first-level lists, include a loader argument...).


Discussion / hints:
Hi @mandresm ,

Thanks for the PR draft, it is an interesting feature!
I would put it into a somewhat wider context, though. There has been another feature on the wishlist, which would go well together with this. My initial thoughts:

First, I would like to connect this to the `base.context` task that already exists instead to create a new, unrelated, task. Mostly because the UI would be clearer, but also because the implementation could take advantage of the similarity of the tasks.
We can add "sub"tasks to `base.context` without changing the UI for the original task. So we could have, for exampel, `base.context.from_yaml` and still have `base.context` unchanged.

Second, there is another useful feature that is needed: It should be possible to set context variables through a dict that is potentially itself stored in the context. This may sound confusing, so here is an example:

```yaml
- base.context:
    user_config:
      foo: 5
# Later, maybe in another file
- base.context:
    foo: 1
    bar: 2
- base.context.from:
    dict: "{{user_config}}"
```
The effect of this extension would be that we are able to better control when context settings take effect. In the example, one can specify a `user_config` that will overwrite the (default) settings at a later stage.

So my suggestions:

1. Keep `base.context` as it is.
2. Add a new `base.context.from` task, which has two possible arguments (for now):
  a. `file`: which reads from a file
  b. `dict`: see above
3. For the file reading, I would not limit to YAML (for the moment). Just implement YAML and give an error if the file cannot be read (and document of course). It would be easy to read other formats (e.g. JSON).

I have started to look at an implementation.
Hi @mandresm ,

Thanks for the PR draft, it is an interesting feature!
I would put it into a somewhat wider context, though. There has been another feature on the wishlist, which would go well together with this. My initial thoughts:

First, I would like to connect this to the `base.context` task that already exists instead to create a new, unrelated, task. Mostly because the UI would be clearer, but also because the implementation could take advantage of the similarity of the tasks.
We can add "sub"tasks to `base.context` without changing the UI for the original task. So we could have, for exampel, `base.context.from_yaml` and still have `base.context` unchanged.

Second, there is another useful feature that is needed: It should be possible to set context variables through a dict that is potentially itself stored in the context. This may sound confusing, so here is an example:

```yaml
- base.context:
    user_config:
      foo: 5
# Later, maybe in another file
- base.context:
    foo: 1
    bar: 2
- base.context.from:
    dict: "{{user_config}}"
```
The effect of this extension would be that we are able to better control when context settings take effect. In the example, one can specify a `user_config` that will overwrite the (default) settings at a later stage.

So my suggestions:

1. Keep `base.context` as it is.
2. Add a new `base.context.from` task, which has two possible arguments (for now):
  a. `file`: which reads from a file
  b. `dict`: see above
3. For the file reading, I would not limit to YAML (for the moment). Just implement YAML and give an error if the file cannot be read (and document of course). It would be easy to read other formats (e.g. JSON).

I have started to look at an implementation.

Fix patch:
diff --git a/CHANGES.txt b/CHANGES.txt
index d02118b..1a816ce 100644
--- a/CHANGES.txt
+++ b/CHANGES.txt
@@ -1,3 +1,11 @@
+ScriptEngine 0.Y.Z
+===================
+
+Features
+--------
+- #77: Add base.context.from (thanks @mandresm for suggesting)
+
+
 ScriptEngine 0.13.1
 ===================
 
diff --git a/docs/sphinx/base-tasks.rst b/docs/sphinx/base-tasks.rst
index 5857488..8660255 100644
--- a/docs/sphinx/base-tasks.rst
+++ b/docs/sphinx/base-tasks.rst
@@ -127,6 +127,60 @@ current values in the ScriptEngine context. This can be used to clean data::
     # mylist is now [3, 4]
 
 
+Context.from task
+-----------------
+This is an extension of ``base.context``. It updates the ScriptEngine context in
+the same way, but it allows to "read" the context update from another source
+instead of explicitly specifying the name-value pairs as task arguments. In
+particular, ``base.context.from`` accepts one of two arguments, ``dict`` or
+``file``. The arguments are mutual exclusive::
+
+    base.context.from:
+        # exactly one of the two arguments:
+        dict: <DICTIONARY>  # optional, mutual exclusive
+        file: <FILE_NAME>  # optional, mutual exclusive
+
+
+If given the ``dict`` argument, the context update is specified by the argument
+value, which must be a dictionary. This may sound rather similar to the standard
+``base.context``, but it allows greater flexibility because the argument value
+can be taken from the context itself. For example, one could implement
+overwriteable default settings using this feature::
+
+    # Let the user set preferred values
+    - base.context:
+        user_config:
+            foo: 5
+    # [... later (could be in another script) ...]
+    # Set default values
+    - base.context:
+        foo: 1
+        bar: 2
+    # Overwrite defaults with user preferences
+    - base.context.from:
+        dict: "{{ user_config  }}"
+    # result: foo==5, bar==2
+
+The ``file`` argument of ``base.context.from`` can be used to read context
+values from a YAML file::
+
+    # data.yml
+    foo: 4
+    bar: 5
+
+    # script.yml
+    - base.context.from:
+        file: data.yml
+
+When running the scripte with ``se script.yml``, the context will contain
+``foo==4`` and ``bar==5``, provided that the file ``data.yml`` can be found in
+the current directory.
+
+The only supported file format for the time being is YAML. The content of the
+file must be a, possibly nested, dictionary (i.e. single values or lists are not
+allowed).
+
+
 Copy task
 ---------
 This task copies the file or directory given by ``src`` to ``dst``. If ``src``
@@ -334,7 +388,7 @@ and::
         False:   No time logging after each task. Does not affect statistic
                  collection.
         'info':  Logging to the info logger.
-        'debug': Logging to the debug logger§
+        'debug': Logging to the debug logger
 
 
 Template task
diff --git a/scriptengine/tasks/base/context.py b/scriptengine/tasks/base/context.py
index 69fe954..1530287 100644
--- a/scriptengine/tasks/base/context.py
+++ b/scriptengine/tasks/base/context.py
@@ -1,6 +1,9 @@
 """Context task for ScriptEngine."""
 
+import yaml
+
 from scriptengine.context import ContextUpdate
+from scriptengine.exceptions import ScriptEngineTaskError, ScriptEngineTaskRunError
 from scriptengine.tasks.core import Task, timed_runner
 
 
@@ -20,3 +23,74 @@ def run(self, context):
         )
         self.log_info(f"Context update: {context_update}")
         return context_update
+
+
+class ContextFrom(Task):
+    """
+    This task updates the context, just as the Context task, but from sources
+    other than direct arguments.
+
+    If the 'dict' argument is given, the argument value must be a dict and is
+    used to update the context, e.g.
+
+    - base.context:
+        upd:
+            foo: 1
+    - base.context.from:
+        dict: "{{upd}}"
+
+    If the 'file' argument is given, the argument value must be a file name and
+    the context is updated from the file context. The file format must be YAML:
+
+    - base.context.from:
+        file: update.yml
+    """
+
+    @timed_runner
+    def run(self, context):
+
+        dict_arg = self.getarg("dict", context, default=False)
+        file_arg = self.getarg("file", context, default=False)
+
+        if dict_arg and file_arg:
+            self.log_error("Task arguments 'dict' and 'file' are mutual exclusive")
+            raise ScriptEngineTaskError
+
+        if dict_arg:
+            self.log_info(f"Context update from dict: {dict_arg}")
+            if not isinstance(dict_arg, dict):
+                self.log_error(
+                    "The 'dict' argument must be a dictionary "
+                    f"(was  a '{type(dict_arg).__name__}')"
+                )
+                raise ScriptEngineTaskRunError
+            context_update_d = dict_arg
+
+        elif file_arg:
+            self.log_info(f"Context update from file: {file_arg}")
+            try:
+                with open(file_arg) as f:
+                    dict_from_file = yaml.load(f, Loader=yaml.SafeLoader)
+            except (FileNotFoundError, PermissionError, IsADirectoryError) as e:
+                self.log_error(e)
+                raise ScriptEngineTaskRunError
+            except yaml.scanner.ScannerError as e:
+                self.log_error("Error reading the file as YAML, error message follows:")
+                self.log_error(f"  {''.join(str(e).splitlines())}")
+                self.log_error(
+                    "Note that only YAML file are supported by base.context.from"
+                )
+                raise ScriptEngineTaskRunError
+            if not isinstance(dict_from_file, dict):
+                self.log_error(
+                    "Reading YAML file succeeded, but the content must be a dict "
+                    f"(was a '{type(dict_from_file).__name__}')"
+                )
+                raise ScriptEngineTaskRunError
+            context_update_d = dict_from_file
+
+        else:
+            self.log_error("Missing argument: either 'dict' or 'file' is needed")
+            raise ScriptEngineTaskError
+
+        return ContextUpdate(context_update_d)
diff --git a/setup.py b/setup.py
index 74f4ac8..5f5e363 100644
--- a/setup.py
+++ b/setup.py
@@ -66,6 +66,7 @@ def get_version(rel_path):
             "base.chdir = scriptengine.tasks.base.chdir:Chdir",
             "base.command = scriptengine.tasks.base.command:Command",
             "base.context = scriptengine.tasks.base.context:Context",
+            "base.context.from = scriptengine.tasks.base.context:ContextFrom",
             "base.copy = scriptengine.tasks.base.file:Copy",
             "base.echo = scriptengine.tasks.base.echo:Echo",
             "base.exit = scriptengine.tasks.base.exit:Exit",


Test patch:
diff --git a/tests/tasks/base/test_context.py b/tests/tasks/base/test_context.py
index b166d11..70b33cc 100644
--- a/tests/tasks/base/test_context.py
+++ b/tests/tasks/base/test_context.py
@@ -1,6 +1,8 @@
+import pytest
 import yaml
 
 from scriptengine.context import ContextUpdate
+from scriptengine.exceptions import ScriptEngineTaskError, ScriptEngineTaskRunError
 from scriptengine.tasks.base.context import Context
 from scriptengine.yaml.parser import parse
 
@@ -51,3 +53,173 @@ def test_context_simple_set():
     ctx += ctx_upd
     assert ctx["foo"] == 1
     assert ctx["bar"] == 2
+
+
+def test_context_from_dict():
+    t = from_yaml(
+        """
+        base.context.from:
+            dict: {'foo': 1, 'bar': 2}
+        """
+    )
+    ctx = {}
+    ctx_upd = t.run(ctx)
+    ctx += ctx_upd
+    assert ctx["foo"] == 1
+    assert ctx["bar"] == 2
+
+
+def test_context_from_context_dict():
+    t1 = from_yaml(
+        """
+        base.context:
+            update:
+                foo: 1
+                bar: 2
+        """
+    )
+    t2 = from_yaml(
+        """
+        base.context.from:
+            dict: '{{update}}'
+        """
+    )
+    ctx = {}
+    ctx_upd = t1.run(ctx)
+    ctx += ctx_upd
+    ctx_upd = t2.run(ctx)
+    ctx += ctx_upd
+    assert ctx["foo"] == 1
+    assert ctx["bar"] == 2
+
+
+def test_context_update_from_context_dict():
+    t1 = from_yaml(
+        """
+        base.context:
+            update:
+                foo: 5
+        """
+    )
+    t2 = from_yaml(
+        """
+        base.context:
+            foo: 1
+            bar: 2
+        """
+    )
+    t3 = from_yaml(
+        """
+        base.context.from:
+            dict: '{{update}}'
+        """
+    )
+    ctx = {}
+    ctx_upd = t1.run(ctx)
+    ctx += ctx_upd
+    ctx_upd = t2.run(ctx)
+    ctx += ctx_upd
+    ctx_upd = t3.run(ctx)
+    ctx += ctx_upd
+    assert ctx["foo"] == 5
+    assert ctx["bar"] == 2
+
+
+def test_context_from_file(tmp_path):
+    f = tmp_path / "f.yml"
+    f.write_text(
+        """
+        foo: 1
+        bar: 2
+        """
+    )
+    t = from_yaml(
+        f"""
+        base.context.from:
+            file: {f}
+        """
+    )
+    ctx = {}
+    ctx_upd = t.run(ctx)
+    ctx += ctx_upd
+    assert ctx["foo"] == 1
+    assert ctx["bar"] == 2
+
+
+def test_context_from_no_args():
+    t = from_yaml(
+        """
+        base.context.from:
+        """
+    )
+    with pytest.raises(ScriptEngineTaskError):
+        t.run({})
+
+
+def test_context_from_double_args():
+    t = from_yaml(
+        """
+        base.context.from:
+            dict: foo
+            file: bar
+        """
+    )
+    with pytest.raises(ScriptEngineTaskError):
+        t.run({})
+
+
+def test_context_from_dict_not_a_dict():
+    t = from_yaml(
+        """
+        base.context.from:
+            dict: [1, 2, 3]
+        """
+    )
+    with pytest.raises(ScriptEngineTaskRunError):
+        t.run({})
+
+
+def test_context_from_file_not_found():
+    t = from_yaml(
+        """
+        base.context.from:
+            file: foo
+        """
+    )
+    with pytest.raises(ScriptEngineTaskRunError):
+        t.run({})
+
+
+def test_context_from_file_not_yaml(tmp_path):
+    f = tmp_path / "f.yml"
+    f.write_text(
+        """
+        @@@
+        """
+    )
+    t = from_yaml(
+        f"""
+        base.context.from:
+            file: {f}
+        """
+    )
+    with pytest.raises(ScriptEngineTaskRunError):
+        t.run({})
+
+
+def test_context_from_file_not_dict(tmp_path):
+    f = tmp_path / "f.yml"
+    f.write_text(
+        """
+        - foo
+        - bar
+        """
+    )
+    t = from_yaml(
+        f"""
+        base.context.from:
+            file: {f}
+        """
+    )
+    with pytest.raises(ScriptEngineTaskRunError):
+        t.run({})