Merge pull request #5605 from wxtim/feature.plus_s_list_of_strings

Added -z option

CHANGES.md

@@ -21,6 +21,10 @@ issue which could cause jobs to fail if this variable became too long.
 
 ### Enhancements
 
+-[#5605](https://github.com/cylc/cylc-flow/pull/5605) - A shorthand for defining
+-a list of strings - Before: `cylc command -s "X=['a', 'bc', 'd']"` - After:
+-`cylc command -z X=a,bc,d`.
+
 [#5537](https://github.com/cylc/cylc-flow/pull/5537) - Allow parameters
 in family names to be split, e.g. `<foo>FAM<bar>`.
 

cylc/flow/option_parsers.py

@@ -22,7 +22,7 @@
     OptionParser,
     Values,
     Option,
-    IndentedHelpFormatter
+    IndentedHelpFormatter,
 )
 import os
 import re
@@ -363,6 +363,15 @@ class CylcOptionParser(OptionParser):
             See `cylc help id` for more details.
     '''))
 
+    CAN_BE_USED_MULTIPLE = (
+        " This option can be used multiple times on the command line.")
+
+    NOTE_PERSIST_ACROSS_RESTARTS = (
+        " NOTE: these settings persist across workflow restarts,"
+        " but can be set again on the \"cylc play\""
+        " command line if they need to be overridden."
+    )
+
     STD_OPTIONS = [
         OptionSettings(
             ['-q', '--quiet'], help='Decrease verbosity.',
@@ -403,13 +412,27 @@ class CylcOptionParser(OptionParser):
                 " Values should be valid Python literals so strings"
                 " must be quoted"
                 " e.g. 'STR=\"string\"', INT=43, BOOL=True."
-                " This option can be used multiple "
-                " times on the command line."
-                " NOTE: these settings persist across workflow restarts,"
-                " but can be set again on the \"cylc play\""
-                " command line if they need to be overridden."
+                + CAN_BE_USED_MULTIPLE
+                + NOTE_PERSIST_ACROSS_RESTARTS
             ),
-            action='append', default=[], dest='templatevars', useif='jset'),
+            action='append', default=[], dest='templatevars', useif='jset'
+        ),
+        OptionSettings(
+            ['-z', '--set-list', '--template-list'],
+            metavar='NAME=VALUE1,VALUE2,...',
+            help=(
+                'Set the value of a Jinja2 template variable in the'
+                ' workflow definition as a comma separated'
+                ' list of Python strings.'
+                ' Values containing commas must be quoted.'
+                " e.g. '+s STR=a,b,c' => ['a', 'b', 'c']"
+                " or '+ s STR=a,\"b,c\"' => ['a', 'b,c']"
+                + CAN_BE_USED_MULTIPLE
+                + NOTE_PERSIST_ACROSS_RESTARTS
+            ),
+            action='append', default=[], dest='templatevars_lists',
+            useif='jset'
+        ),
         OptionSettings(
             ['--set-file'], metavar='FILE',
             help=(
@@ -418,9 +441,7 @@ class CylcOptionParser(OptionParser):
                 " pairs (one per line)."
                 " As with --set values should be valid Python literals "
                 " so strings must be quoted e.g. STR='string'."
-                " NOTE: these settings persist across workflow restarts,"
-                " but can be set again on the \"cylc play\""
-                " command line if they need to be overridden."
+                + NOTE_PERSIST_ACROSS_RESTARTS
             ),
             action='store', default=None, dest='templatevars_file',
             useif='jset'

cylc/flow/scheduler.py

@@ -140,7 +140,7 @@
     TASK_STATUS_RUNNING,
     TASK_STATUS_WAITING,
     TASK_STATUS_FAILED)
-from cylc.flow.templatevars import load_template_vars
+from cylc.flow.templatevars import get_template_vars
 from cylc.flow.util import cli_format
 from cylc.flow.wallclock import (
     get_current_time_string,
@@ -278,10 +278,7 @@ def __init__(self, reg: str, options: Values) -> None:
         self.id = self.tokens.id
         self.uuid_str = str(uuid4())
         self.options = options
-        self.template_vars = load_template_vars(
-            self.options.templatevars,
-            self.options.templatevars_file
-        )
+        self.template_vars = get_template_vars(self.options)
 
         # mutable defaults
         self._profile_amounts = {}

cylc/flow/scripts/check_versions.py

@@ -48,7 +48,7 @@
 from cylc.flow.id_cli import parse_id
 from cylc.flow.platforms import get_platform, get_host_from_platform
 from cylc.flow.remote import construct_ssh_cmd
-from cylc.flow.templatevars import load_template_vars
+from cylc.flow.templatevars import get_template_vars
 from cylc.flow.terminal import cli_function
 
 if TYPE_CHECKING:
@@ -87,7 +87,7 @@ def main(_, options: 'Values', *ids) -> None:
         workflow_id,
         flow_file,
         options,
-        load_template_vars(options.templatevars, options.templatevars_file))
+        get_template_vars(options))
 
     platforms = {
         config.get_config(['runtime', name, 'platform'])

cylc/flow/scripts/diff.py

@@ -39,7 +39,7 @@
     icp_option,
 )
 from cylc.flow.config import WorkflowConfig
-from cylc.flow.templatevars import load_template_vars
+from cylc.flow.templatevars import get_template_vars
 from cylc.flow.terminal import cli_function
 
 if TYPE_CHECKING:
@@ -155,9 +155,7 @@ def main(parser: COP, options: 'Values', workflow_id1: str, workflow_id2: str):
     if workflow_file_1_ == workflow_file_2_:
         parser.error("You can't diff a single workflow.")
     print(f"Parsing {workflow_id_1} ({workflow_file_1_})")
-    template_vars = load_template_vars(
-        options.templatevars, options.templatevars_file
-    )
+    template_vars = get_template_vars(options)
     config1 = WorkflowConfig(
         workflow_id_1, workflow_file_1_, options, template_vars
     ).cfg

cylc/flow/scripts/view.py

@@ -35,7 +35,7 @@
     CylcOptionParser as COP,
 )
 from cylc.flow.parsec.fileparse import read_and_proc
-from cylc.flow.templatevars import load_template_vars
+from cylc.flow.templatevars import get_template_vars
 from cylc.flow.terminal import cli_function
 
 if TYPE_CHECKING:
@@ -114,7 +114,6 @@ async def _main(options: 'Values', workflow_id: str) -> None:
         src=True,
         constraint='workflows',
     )
-
     # read in the flow.cylc file
     viewcfg = {
         'mark': options.mark,
@@ -128,7 +127,7 @@ async def _main(options: 'Values', workflow_id: str) -> None:
     }
     for line in read_and_proc(
         flow_file,
-        load_template_vars(options.templatevars, options.templatevars_file),
+        get_template_vars(options),
         viewcfg=viewcfg,
         opts=options,
     ):

cylc/flow/templatevars.py

@@ -17,15 +17,21 @@
 
 from ast import literal_eval
 from optparse import Values
-from typing import TYPE_CHECKING, Any, Dict
+from typing import Any, Dict, List, Optional, Set, Union
 
+from cylc.flow import LOG
 from cylc.flow.exceptions import InputError
 from cylc.flow.rundb import CylcWorkflowDAO
 from cylc.flow.workflow_db_mgr import WorkflowDatabaseManager
 from cylc.flow.workflow_files import WorkflowFiles
 
-if TYPE_CHECKING:
-    from pathlib import Path
+from pathlib import Path
+
+
+OVERWRITE_WARNING = (
+    'Template variable {} redefined:'
+    ' the previous value will be overwritten.'
+)
 
 
 def get_template_vars_from_db(run_dir: 'Path') -> dict:
@@ -77,22 +83,102 @@ def eval_var(var):
         ) from None
 
 
-def load_template_vars(template_vars=None, template_vars_file=None):
+def parse_string_list(stringlist: str) -> List:
+    """Parse a comma separated string list into a Python string list.
+
+    Examples:
+        >>> parse_string_list('a,b,c')
+        ['a', 'b', 'c']
+        >>> parse_string_list('a,"b,b",c')
+        ['a', 'b,b', 'c']
+        >>> parse_string_list("a,'b,b','c'")
+        ['a', 'b,b', 'c']
+    """
+    list_ = []
+    in_quotes = False
+    buffer = ''
+    for char in stringlist:
+        if char in {'"', "'"}:
+            in_quotes = not in_quotes
+        elif not in_quotes and char == ',':
+            list_.append(buffer)
+            buffer = ''
+        else:
+            buffer += char
+    list_.append(buffer)
+    return list_
+
+
+def load_template_vars(
+    template_vars: Optional[List[str]] = None,
+    template_vars_file: Union[Path, str, None] = None,
+    templatevars_lists: Optional[List[str]] = None
+) -> Dict[str, Any]:
     """Load template variables from key=value strings."""
-    res = {}
+    keys: Set[str] = set()
+    invalid_vars: str = ''
+
+    # Parse Template vars set by file (-S)
+    file_tvars: Dict[str, Any] = {}
     if template_vars_file:
         with open(template_vars_file, 'r') as handle:
             for line in handle:
                 line = line.strip().split("#", 1)[0]
                 if not line:
                     continue
-                key, val = line.split("=", 1)
-                res[key.strip()] = eval_var(val.strip())
+                try:
+                    key, val = line.split("=", 1)
+                except ValueError:
+                    invalid_vars += f'\n * {line}'
+                    continue
+                file_tvars[key.strip()] = eval_var(val.strip())
+                keys.add(key.strip())
+
+    cli_tvars: Dict[str, Any] = {}
+    tvars_lists: Dict[str, str] = {}
+    for input_, result, func in (
+        (template_vars, cli_tvars, eval_var),
+        (templatevars_lists, tvars_lists, parse_string_list)
+    ):
+        for pair in input_ or []:
+            # Handle ValueError
+            try:
+                key, val = pair.split("=", 1)
+            except ValueError:
+                invalid_vars += f'\n * {pair}'
+                continue
+            key, val = key.strip(), val.strip()
+            if key in result:
+                LOG.warning(OVERWRITE_WARNING.format(key))
+            result[key] = func(val)
+            keys.add(key)
+
+    # Raise an error if there are any args without the form key=value.:
+    if invalid_vars:
+        raise InputError(
+            'Template variables must be set with key=value(s):'
+            + invalid_vars
+        )
+
+    # Explicitly record which source of tvars over-rides which.
+    res = {}
+    badkeys = ''
+    for key in keys:
+        if key in cli_tvars and key in tvars_lists:
+            badkeys += (
+                f"\n * {key}={cli_tvars[key]} and {key}={tvars_lists[key]}")
+        else:
+            res[key] = cli_tvars.get(
+                key, tvars_lists.get(
+                    key, file_tvars.get(key)))
+
+    # Raise an error if there are any key-clashes between tvars and tvars_list
+    if badkeys:
+        raise InputError(
+            "You can't set the same template variable with both -s and -z:"
+            + badkeys
+        )
 
-    if template_vars:
-        for pair in template_vars:
-            key, val = pair.split("=", 1)
-            res[key.strip()] = eval_var(val.strip())
     return res
 
 
@@ -106,4 +192,8 @@ def get_template_vars(options: Values) -> Dict[str, Any]:
     Returns:
         template_vars: Template variables to give to a Cylc config.
     """
-    return load_template_vars(options.templatevars, options.templatevars_file)
+    return load_template_vars(
+        options.templatevars,
+        options.templatevars_file,
+        options.templatevars_lists
+    )

tests/integration/test_template_variables_cli.py

@@ -0,0 +1,39 @@
+#!/usr/bin/env python3
+
+# THIS FILE IS PART OF THE CYLC WORKFLOW ENGINE.
+# Copyright (C) NIWA & British Crown (Met Office) & Contributors.
+#
+# This program is free software: you can redistribute it and/or modify
+# it under the terms of the GNU General Public License as published by
+# the Free Software Foundation, either version 3 of the License, or
+# (at your option) any later version.
+#
+# This program is distributed in the hope that it will be useful,
+# but WITHOUT ANY WARRANTY; without even the implied warranty of
+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+# GNU General Public License for more details.
+#
+# You should have received a copy of the GNU General Public License
+# along with this program.  If not, see <http://www.gnu.org/licenses/>.
+
+from cylc.flow.scripts.view import get_option_parser, _main as view
+from cylc.flow.option_parsers import Options
+
+
+async def test_list_tvars(tmp_path, capsys):
+    """View shows that lists of comma separated args are converted into
+    strings:
+    """
+    (tmp_path / 'flow.cylc').write_text(
+        '#!jinja2\n'
+        '{% for i in FOO %}\n'
+        '# {{i}} is string: {{i is string}}\n'
+        '{% endfor %}\n'
+    )
+    options = Options(get_option_parser())()
+    options.jinja2 = True
+    options.templatevars_lists = ['FOO="w,x",y,z']
+    await view(options, str(tmp_path))
+    result = capsys.readouterr().out.split('\n')
+    for string in ['w,x', 'y', 'z']:
+        assert f'# {string} is string: True' in result