Review Feedback HO

Co-authored-by: Hilary James Oliver <[email protected]>

Rebase Tidy

cylc/flow/cfgspec/globalcfg.py

@@ -372,13 +372,12 @@
         # Symlink Dirs
         with Conf('symlink dirs',  # noqa: SIM117 (keep same format)
                   desc="""
-            Define directories to be moved, symlinks from the the original
-            ``$HOME/cylc-run`` directories will be created.
+            Configure alternate workflow run directory locations. Symlinks from
+            the the standard ``$HOME/cylc-run`` locations will be created.
         """):
             with Conf('<install target>'):
                 Conf('run', VDR.V_STRING, None, desc="""
-                    Specifies the directory where the workflow run directories
-                    are created. If specified, the workflow run directory will
+                    If specified, the workflow run directory will
                     be created in ``<run dir>/cylc-run/<workflow-name>`` and a
                     symbolic link will be created from
                     ``$HOME/cylc-run/<workflow-name>``.
@@ -388,7 +387,6 @@
                     installed into this directory.
                 """)
                 Conf('log', VDR.V_STRING, None, desc="""
-                    Specifies the directory where log directories are created.
                     If specified the workflow log directory will be created in
                     ``<log dir>/cylc-run/<workflow-name>/log`` and a symbolic
                     link will be created from
@@ -397,17 +395,15 @@
                     ``$HOME/cylc-run/<workflow-name>/log``.
                 """)
                 Conf('share', VDR.V_STRING, None, desc="""
-                    Specifies the directory where share directories are
-                    created. If specified the workflow share directory will be
+                    If specified the workflow share directory will be
                     created in ``<share dir>/cylc-run/<workflow-name>/share``
                     and a symbolic link will be created from
                     ``<$HOME/cylc-run/<workflow-name>/share``. If not specified
                     the workflow share directory will be created in
                     ``$HOME/cylc-run/<workflow-name>/share``.
                 """)
                 Conf('share/cycle', VDR.V_STRING, None, desc="""
-                    Specifies the directory where share/cycle directories are
-                    created. If specified the workflow share/cycle directory
+                    If specified the workflow share/cycle directory
                     will be created in
                     ``<share/cycle dir>/cylc-run/<workflow-name>/share/cycle``
                     and a symbolic link will be created from
@@ -416,7 +412,6 @@
                     created in ``$HOME/cylc-run/<workflow-name>/share/cycle``.
                 """)
                 Conf('work', VDR.V_STRING, None, desc="""
-                    Specifies the directory where work directories are created.
                     If specified the workflow work directory will be created in
                     ``<work dir>/cylc-run/<workflow-name>/work`` and a symbolic
                     link will be created from

cylc/flow/pathutil.py

@@ -19,7 +19,7 @@
 from pathlib import Path
 import re
 from shutil import rmtree
-from typing import Dict, Iterable, Set, Union, Optional, OrderedDict, Any
+from typing import Dict, Iterable, Set, Union, Optional, Any
 
 from cylc.flow import LOG
 from cylc.flow.cfgspec.glbl_cfg import glbl_cfg
@@ -131,7 +131,7 @@ def make_workflow_run_tree(workflow):
 
 def make_localhost_symlinks(
     rund: Union[Path, str],
-    named_sub_dir: str, 
+    named_sub_dir: str,
     symlink_conf: Optional[Dict[str, Dict[str, str]]] = None
 ) -> Dict[str, Union[Path, str]]:
     """Creates symlinks for any configured symlink dirs from glbl_cfg.
@@ -190,7 +190,7 @@ def get_dirs_to_symlink(
         dirs_to_symlink: [directory: symlink_path]
     """
     dirs_to_symlink: Dict[str, Any] = {}
-    if not symlink_conf:
+    if symlink_conf is None:
         symlink_conf = glbl_cfg().get(['install', 'symlink dirs'])
     if install_target not in symlink_conf.keys():
         return dirs_to_symlink
@@ -199,7 +199,7 @@ def get_dirs_to_symlink(
         dirs_to_symlink['run'] = os.path.join(base_dir, 'cylc-run', flow_name)
     for dir_ in ['log', 'share', 'share/cycle', 'work']:
         link = symlink_conf[install_target].get(dir_, None)
-        if (not link) is None or link == base_dir:
+        if (not link) or link == base_dir:
             continue
         dirs_to_symlink[dir_] = os.path.join(link, 'cylc-run', flow_name, dir_)
     return dirs_to_symlink

cylc/flow/workflow_files.py

@@ -43,8 +43,9 @@
     PlatformLookupError,
     ServiceFileError,
     TaskRemoteMgmtError,
-    WorkflowFilesError,
-    handle_rmtree_err
+    handle_rmtree_err,
+    UserInputError,
+    WorkflowFilesError
 )
 from cylc.flow.pathutil import (
     expand_path,
@@ -227,7 +228,7 @@ class Install:
         SHARE_CYCLE_DIR, SHARE_DIR, LOG_DIR, WORK_DIR, ''
     ])
     """The paths of the symlink dirs that may be set in
-    global.cylc[symlink dirs], relative to the run dir
+    global.cylc[install]symlink dirs, relative to the run dir
     ('' represents the run dir)."""
 
 
@@ -584,7 +585,7 @@ def register(
     if not is_installed(get_workflow_run_dir(flow_name)):
         symlinks_created = make_localhost_symlinks(
             get_workflow_run_dir(flow_name), flow_name)
-        if bool(symlinks_created):
+        if symlinks_created:
             for src, dst in symlinks_created.items():
                 LOG.info(f"Symlink created from {src} to {dst}")
     # Create service dir if necessary.
@@ -608,11 +609,7 @@ def is_installed(rund: Union[Path, str]) -> bool:
     rund = Path(rund)
     cylc_install_dir = Path(rund, WorkflowFiles.Install.DIRNAME)
     alt_cylc_install_dir = Path(rund.parent, WorkflowFiles.Install.DIRNAME)
-    if cylc_install_dir.is_dir():
-        return True
-    elif alt_cylc_install_dir.is_dir():
-        return True
-    return False
+    return cylc_install_dir.is_dir() or alt_cylc_install_dir.is_dir()
 
 
 def _clean_check(reg: str, run_dir: Path) -> None:
@@ -1316,7 +1313,7 @@ def install_workflow(
     symlinks_created = make_localhost_symlinks(
         rundir, named_run, symlink_conf=cli_symlink_dirs)
     install_log = _get_logger(rundir, 'cylc-install')
-    if bool(symlinks_created) is True:
+    if symlinks_created:
         for src, dst in symlinks_created.items():
             install_log.info(f"Symlink created from {src} to {dst}")
     try:
@@ -1528,15 +1525,22 @@ def get_sym_dirs(symlink_dirs: str) -> Dict[str, Dict[str, Any]]:
     symdict: Dict[str, Dict[str, Any]] = {'localhost': {'run': None}}
     if symlink_dirs == "":
         return symdict
-    symlist = symlink_dirs.replace(" ", "").strip(',').split(',')
+    symlist = symlink_dirs.strip(',').split(',')
     for pair in symlist:
         try:
             key, val = pair.split("=")
+            key = key.strip()
         except ValueError:
-            continue
+            raise UserInputError(
+                'There is an error in --symlink-dirs option:'
+                f' {pair}. Try entering option in the form '
+                '--symlink-dirs=\'log=$DIR, share=$DIR2, ...\''
+            )
         if key not in ['run', 'log', 'share', 'share/cycle', 'work']:
-            raise WorkflowFilesError(
-                f"{key} not a valid entry for --symlink-dirs"
+            raise UserInputError(
+                f"{key} not a valid entry for --symlink-dirs. "
+                "Configurable symlink dirs are: 'run', 'log', 'share',"
+                " 'share/cycle', 'work'"
             )
         symdict['localhost'][key] = val.strip() or None
 

tests/functional/cylc-clean/02-targeted.t

@@ -28,14 +28,15 @@ SYM_NAME="$(mktemp -u)"
 SYM_NAME="sym-${SYM_NAME##*tmp.}"
 
 create_test_global_config "" "
-[symlink dirs]
-    [[localhost]]
-        run = ${TEST_DIR}/${SYM_NAME}/run
-        log = ${TEST_DIR}/${SYM_NAME}/other
-        share = ${TEST_DIR}/${SYM_NAME}/other
-        work = ${TEST_DIR}/${SYM_NAME}/other
-        # Need to override any symlink dirs set in global-tests.cylc:
-        share/cycle =
+[install]
+    [[symlink dirs]]
+        [[[localhost]]]
+            run = ${TEST_DIR}/${SYM_NAME}/run
+            log = ${TEST_DIR}/${SYM_NAME}/other
+            share = ${TEST_DIR}/${SYM_NAME}/other
+            work = ${TEST_DIR}/${SYM_NAME}/other
+            # Need to override any symlink dirs set in global-tests.cylc:
+            share/cycle =
 "
 init_workflow "${TEST_NAME_BASE}" << __FLOW__
 [scheduler]

tests/unit/test_workflow_files.py

@@ -29,6 +29,7 @@
     CylcError,
     ServiceFileError,
     TaskRemoteMgmtError,
+    UserInputError,
     WorkflowFilesError
 )
 from cylc.flow.option_parsers import Options
@@ -1450,13 +1451,16 @@ def test_check_flow_file_symlink(
 @pytest.mark.parametrize(
     'symlink_dirs, err_msg, expected',
     [
-        ('log=$shortbread, share= $bourbon,share/cycle= $digestive, ', 'None',
-            {'localhost': {
-                'run': None,
-                'log': "$shortbread",
-                'share': '$bourbon',
-                'share/cycle': '$digestive'
-            }}
+        ('log=$shortbread, share= $bourbon,share/cycle= $digestive, ',
+         "There is an error in --symlink-dirs option:",
+            None
+         ),
+        ('log=$shortbread share= $bourbon share/cycle= $digestive ',
+         "There is an error in --symlink-dirs option:"
+         " log=$shortbread share= $bourbon share/cycle= $digestive . "
+         "Try entering option in the form --symlink-dirs="
+         "'log=$DIR, share=$DIR2, ...'",
+            None
          ),
         ('run=$NICE, log= $Garibaldi, share/cycle=$RichTea', 'None',
             {'localhost': {
@@ -1479,7 +1483,7 @@ def test_get_sym_dirs(
     """Test get_sym_dirs returns dict or correctly raises errors on cli symlink
         dir options"""
     if err_msg != 'None':
-        with pytest.raises(WorkflowFilesError) as exc:
+        with pytest.raises(UserInputError) as exc:
             get_sym_dirs(symlink_dirs)
             assert(err_msg) in str(exc)