Repository: fractal-analytics-platform/fractal-client (language: python)

Issue:
Reminder: tasks with digits-only names will break their by-name identification
That is, if a task has name `"123"`, it's impossible to know whether this is its name or its ID (e.g. in `task edit`). This is a problem of fractal-client only, and it can be fixed easily by changing the arguments (and removing the current `task_id_or_name` one).


Discussion / hints:
Plan for `fractal task edit`:
* Remove `task_name_or_id` positional argument
* Introduce `--name` and `--id` keyword arguments
* Fail if none of the two is provided
* Fail if both are provided
* Fail if `--id` is provided and also `--version` 

Plan for `fractal workflow add-task`:
* Remove  `task_name_or_id` positional argument
* Introduce `--task-name` and `--task-id` keyword arguments
* Proceed as above

Fix patch:
diff --git a/CHANGELOG.md b/CHANGELOG.md
index 6ea4d4ff6..b240707b4 100644
--- a/CHANGELOG.md
+++ b/CHANGELOG.md
@@ -15,6 +15,7 @@
     * Add `--username/--new-username` to `fractal user` subcommands (\#493).
     * Remove `--private` option for `fractal task collect` (\#493).
     * Add `--version` to `fractal task {new,edit}` subcommands (\#493).
+    * Split `task-id-or-name` argument of `task edit` and `workflow add-task` into two arguments (\#504)
 * Package and repository:
     * Fix a bug in tests, by starting the fractal-server FastAPI app in a more standard way (\#481).
     * Require pydantic version to be `>=1.10.8` (\#486, \#490).
diff --git a/fractal/cmd/__init__.py b/fractal/cmd/__init__.py
index 23e588cff..50556805d 100644
--- a/fractal/cmd/__init__.py
+++ b/fractal/cmd/__init__.py
@@ -169,7 +169,8 @@ async def task(
         iface = await post_task(client, batch=batch, **function_kwargs)
     elif subcmd == "edit":
         parameters = [
-            "task_id_or_name",
+            "id",
+            "name",
             "version",
             "new_name",
             "new_command",
@@ -182,7 +183,7 @@ async def task(
         function_kwargs = get_kwargs(parameters, kwargs)
         iface = await patch_task(client, **function_kwargs)
     elif subcmd == "delete":
-        parameters = ["task_id_or_name"]
+        parameters = ["id", "name", "version"]
         function_kwargs = get_kwargs(parameters, kwargs)
         iface = await delete_task(client, **function_kwargs)
     else:
@@ -220,8 +221,9 @@ async def workflow(
         parameters = [
             "project_id",
             "workflow_id",
-            "task_id_or_name",
-            "version",
+            "task_id",
+            "task_name",
+            "task_version",
             "order",
             "args_file",
             "meta_file",
diff --git a/fractal/cmd/_task.py b/fractal/cmd/_task.py
index df5ae5064..63f1f48a7 100644
--- a/fractal/cmd/_task.py
+++ b/fractal/cmd/_task.py
@@ -132,7 +132,8 @@ async def post_task(
 async def patch_task(
     client: AuthClient,
     *,
-    task_id_or_name: str,
+    id: Optional[int] = None,
+    name: Optional[str] = None,
     version: Optional[str] = None,
     new_name: Optional[str] = None,
     new_command: Optional[str] = None,
@@ -143,15 +144,16 @@ async def patch_task(
     meta_file: Optional[str] = None,
 ) -> BaseInterface:
 
-    try:
-        task_id = int(task_id_or_name)
+    if id:
         if version:
-            logging.warning("Task Version is ignored because Task ID provided")
-    except ValueError:
-        task_name = task_id_or_name
+            logging.error(
+                "Too many arguments: cannot provide both `id` and `version`."
+            )
+            sys.exit(1)
+    else:
         try:
-            task_id = await get_task_id_from_cache(
-                client=client, task_name=task_name, version=version
+            id = await get_task_id_from_cache(
+                client=client, task_name=name, version=version
             )
         except FractalCacheError as e:
             print(e)
@@ -180,9 +182,7 @@ async def patch_task(
     if not payload:
         return PrintInterface(retcode=1, data="Nothing to update")
 
-    res = await client.patch(
-        f"{settings.BASE_URL}/task/{task_id}", json=payload
-    )
+    res = await client.patch(f"{settings.BASE_URL}/task/{id}", json=payload)
     new_task = check_response(res, expected_status_code=200, coerce=TaskRead)
     return RichJsonInterface(retcode=0, data=new_task.dict())
 
@@ -190,7 +190,9 @@ async def patch_task(
 async def delete_task(
     client: AuthClient,
     *,
-    task_id_or_name: str,
+    id: Optional[int] = None,
+    name: Optional[str] = None,
+    version: Optional[str] = None,
 ) -> PrintInterface:
 
     raise NotImplementedError("task_delete")
diff --git a/fractal/cmd/_workflow.py b/fractal/cmd/_workflow.py
index 86c229354..29706b087 100644
--- a/fractal/cmd/_workflow.py
+++ b/fractal/cmd/_workflow.py
@@ -1,4 +1,5 @@
 import json
+import logging
 import sys
 from pathlib import Path
 from typing import Optional
@@ -81,20 +82,26 @@ async def post_workflowtask(
     *,
     project_id: int,
     workflow_id: int,
-    task_id_or_name: str,
+    task_id: Optional[int] = None,
+    task_name: Optional[str] = None,
+    task_version: Optional[str] = None,
     batch: bool = False,
-    version: Optional[str] = None,
     order: Optional[int] = None,
     args_file: Optional[str] = None,
     meta_file: Optional[str] = None,
 ) -> RichJsonInterface:
 
-    try:
-        task_id = int(task_id_or_name)
-    except ValueError:
+    if task_id:
+        if task_version:
+            logging.error(
+                "Too many arguments: cannot provide both "
+                "`task_id` and `task_version`."
+            )
+            sys.exit(1)
+    else:
         try:
             task_id = await get_task_id_from_cache(
-                client=client, task_name=task_id_or_name, version=version
+                client=client, task_name=task_name, version=task_version
             )
         except FractalCacheError as e:
             print(e)
diff --git a/fractal/parser.py b/fractal/parser.py
index 81c1cee40..82f538367 100644
--- a/fractal/parser.py
+++ b/fractal/parser.py
@@ -373,11 +373,24 @@
     argument_default=ap.SUPPRESS,
     allow_abbrev=False,
 )
-task_edit_parser.add_argument(
-    "task_id_or_name", help="ID or name of task to edit", type=str
+
+task_edit_id_or_name_group = task_edit_parser.add_mutually_exclusive_group(
+    required=True
+)
+task_edit_id_or_name_group.add_argument(
+    "--id", help="ID of the task to edit", type=int
 )
+task_edit_id_or_name_group.add_argument(
+    "--name", help="Name of the task to edit", type=str
+)
+
 task_edit_parser.add_argument(
-    "--version", type=str, help="Version of the task to edit"
+    "--version",
+    type=str,
+    help=(
+        "Version of the task to edit "
+        "(only accepted in combination with --name)"
+    ),
 )
 task_edit_parser.add_argument("--new-name", help="New task name", type=str)
 task_edit_parser.add_argument(
@@ -423,8 +436,22 @@
     argument_default=ap.SUPPRESS,
     allow_abbrev=False,
 )
+task_delete_id_or_name_group = task_delete_parser.add_mutually_exclusive_group(
+    required=True
+)
+task_delete_id_or_name_group.add_argument(
+    "--id", help="ID of the task to delete", type=int
+)
+task_delete_id_or_name_group.add_argument(
+    "--name", help="Name of the task to delete", type=str
+)
 task_delete_parser.add_argument(
-    "task_id_or_name", help="ID or name of task to delete", type=str
+    "--version",
+    type=str,
+    help=(
+        "Version of the task to delete "
+        "(only accepted in combination with --name)"
+    ),
 )
 
 
@@ -511,11 +538,24 @@
     type=int,
     help="Workflow ID",
 )
-workflow_add_task_parser.add_argument(
-    "task_id_or_name", help="ID or name of the new task", type=str
+
+workflow_add_task_id_or_name_group = (
+    workflow_add_task_parser.add_mutually_exclusive_group(required=True)
 )
+workflow_add_task_id_or_name_group.add_argument(
+    "--task-id", help="ID of the task to add", type=int
+)
+workflow_add_task_id_or_name_group.add_argument(
+    "--task-name", help="Name of the task to add", type=str
+)
+
 workflow_add_task_parser.add_argument(
-    "--version", type=str, help="Version of the new task"
+    "--task-version",
+    type=str,
+    help=(
+        "Version of task to add "
+        "(only accepted in combination with --task-name)"
+    ),
 )
 workflow_add_task_parser.add_argument(
     "--order", help="Order of this task within the workflow's task list"


Test patch:
diff --git a/tests/test_task.py b/tests/test_task.py
index 4e45521dd..6af3c0c3a 100644
--- a/tests/test_task.py
+++ b/tests/test_task.py
@@ -176,44 +176,61 @@ async def test_task_edit(
     task.show()
     assert task.retcode == 0
     task_id = task.data["id"]
-    NEW = "new"
+    NEW_NAME = "1234"
 
     # Test that regular user is not authorized
     with pytest.raises(SystemExit):
-        res = await invoke(f"task edit {task_id} --new-name {NEW}")
+        res = await invoke(f"task edit {task_id} --new-name {NEW_NAME}")
 
     # Test successful edit of string attributes
-    NAME = "new-name"
-    res = await invoke_as_superuser(f"task edit {task_id} --new-name {NAME}")
-    assert res.data["name"] == NAME
-    assert res.retcode == 0
-    res = await invoke_as_superuser(f"task edit {task_id} --new-command {NEW}")
-    assert res.data["command"] == NEW
+    res = await invoke_as_superuser(
+        f"task edit --id {task_id} --new-name {NEW_NAME}"
+    )
+    assert res.data["name"] == NEW_NAME
     assert res.retcode == 0
 
-    # Test version ignored
+    NEW_COMMAND = "run"
     res = await invoke_as_superuser(
-        f"task edit {task_id} --version 1.2.3.4.5.6"
+        f"task edit --id {task_id} --new-command {NEW_COMMAND}"
     )
+    assert res.data["command"] == NEW_COMMAND
+    assert res.retcode == 0
+
+    # Test fail with no task_id nor task_name
+    with pytest.raises(SystemExit):
+        res = await invoke_as_superuser("task edit")
+    # Test fail with both task_id and task_name
+    with pytest.raises(SystemExit):
+        res = await invoke_as_superuser(
+            f"task edit --id {task_id} --name {task.data['name']}"
+        )
+    # Test fail with both task_id and task_version
+    with pytest.raises(SystemExit):
+        res = await invoke_as_superuser(
+            f"task edit --id {task_id} --version 1.2.3.4.5.6"
+        )
     assert caplog.records[-1].msg == (
-        "Task Version is ignored because Task ID provided"
+        "Too many arguments: cannot provide both `id` and `version`."
     )
-    assert res.retcode == 1
-    assert res.data == "Nothing to update"
 
+    NEW_TYPE = "zip"
     # Test regular updates (both by id and name)
     res = await invoke_as_superuser(
-        f"task edit {task_id} --new-input-type {NEW}"
+        f"task edit --id {task_id} --new-input-type {NEW_TYPE}"
     )
-    assert res.data["input_type"] == NEW
+    assert res.data["input_type"] == NEW_TYPE
     assert res.retcode == 0
     res = await invoke_as_superuser(
-        f"task edit {NAME} --new-output-type {NEW}"
+        f"task edit --name {NEW_NAME} --new-output-type {NEW_TYPE}"
     )
-    assert res.data["output_type"] == NEW
+    assert res.data["output_type"] == NEW_TYPE
     assert res.retcode == 0
-    res = await invoke_as_superuser(f"task edit {task_id} --new-version {NEW}")
-    assert res.data["version"] == NEW
+
+    NEW_VERSION = "3.14"
+    res = await invoke_as_superuser(
+        f"task edit --id {task_id} --new-version {NEW_VERSION}"
+    )
+    assert res.data["version"] == NEW_VERSION
     assert res.retcode == 0
 
     # Test regular update by name, after deleting cache
@@ -222,7 +239,7 @@ async def test_task_edit(
     cache_file.unlink(missing_ok=True)
     NEW_TYPE = "something"
     res = await invoke_as_superuser(
-        f"task edit {NAME} --new-output-type {NEW_TYPE}"
+        f"task edit --name {NEW_NAME} --new-output-type {NEW_TYPE}"
     )
     assert res.data["output_type"] == NEW_TYPE
     assert res.retcode == 0
@@ -232,39 +249,42 @@ async def test_task_edit(
     NEW_TYPE = "something-here"
     with pytest.raises(SystemExit):
         res = await invoke_as_superuser(
-            f"task edit INVALID_NAME --new-output-type {NEW_TYPE}"
+            f"task edit --name INVALID_NAME --new-output-type {NEW_TYPE}"
         )
 
     # Test regular update by name, after creating an invalid cache
     with cache_file.open("w") as f:
         json.dump([], f)
     NEW_TYPE = "something-else"
-    debug(f"task edit {NAME} --new-output-type {NEW_TYPE}")
+    debug(f"task edit --name {NEW_NAME} --new-output-type {NEW_TYPE}")
     res = await invoke_as_superuser(
-        f"task edit {NAME} --new-output-type {NEW_TYPE}"
+        f"task edit --name {NEW_NAME} --new-output-type {NEW_TYPE}"
     )
     assert res.data["output_type"] == NEW_TYPE
     assert res.retcode == 0
 
     # Test `file not found` errors
+    NEW_FILE = "foo.json"
     with pytest.raises(FileNotFoundError):
         res = await invoke_as_superuser(
-            f"task edit {task_id} --default-args-file {NEW}"
+            f"task edit --id {task_id} --default-args-file {NEW_FILE}"
         )
     with pytest.raises(FileNotFoundError):
-        await invoke_as_superuser(f"task edit {task_id} --meta-file {NEW}")
+        await invoke_as_superuser(
+            f"task edit --id {task_id} --meta-file {NEW_FILE}"
+        )
 
     # Test successful edit of dictionary attributes
     args_file = str(testdata_path / "task_edit_json/default_args.json")
     res = await invoke_as_superuser(
-        f"task edit {task_id} --default-args-file {args_file}"
+        f"task edit --id {task_id} --default-args-file {args_file}"
     )
     debug(res)
     debug(res.data)
     assert res.retcode == 0
     meta_file = str(testdata_path / "task_edit_json/meta.json")
     res = await invoke_as_superuser(
-        f"task edit {task_id} --meta-file {meta_file}"
+        f"task edit --id {task_id} --meta-file {meta_file}"
     )
     debug(res)
     debug(res.data)
@@ -280,8 +300,8 @@ async def test_task_delete(register_user, invoke):
     assert res.retcode == 0
     task_id = res.data["id"]
     with pytest.raises(NotImplementedError):
-        debug(f"task delete {task_id}")
-        res = await invoke(f"task delete {task_id}")
+        debug(f"task delete --id {task_id}")
+        res = await invoke(f"task delete --id {task_id}")
 
 
 async def test_task_list(register_user, invoke, testdata_path):
diff --git a/tests/test_workflow.py b/tests/test_workflow.py
index d6905ac64..73d4b5a27 100644
--- a/tests/test_workflow.py
+++ b/tests/test_workflow.py
@@ -130,6 +130,7 @@ async def test_workflow_list_when_two_projects_exist(register_user, invoke):
 
 
 async def test_workflow_add_task(
+    caplog,
     invoke,
     register_user,
     task_factory,
@@ -160,11 +161,28 @@ async def test_workflow_add_task(
     with meta_file.open("w") as f:
         json.dump(META, f)
 
+    cmd = f"workflow add-task {project_id} {wf.id}"
+    # Test fail with no task_id nor task_name
+    with pytest.raises(SystemExit):
+        res = await invoke(cmd)
+    # Test fail with both task_id and task_name
+    with pytest.raises(SystemExit):
+        res = await invoke(f"{cmd} --task-id {t.id} --task-name {t.name}")
+    # Test fail with both task_id and version
+    with pytest.raises(SystemExit):
+        res = await invoke(
+            f"{cmd} --task-id {t.id} --task-version 1.2.3.4.5.6"
+        )
+    assert caplog.records[-1].msg == (
+        "Too many arguments: cannot provide both `task_id` and `task_version`."
+    )
+
     cmd = (
-        f"workflow add-task {project_id} {wf.id} {t.id} "
+        f"{cmd} --task-id {t.id} "
         f"--args-file {args_file} --meta-file {meta_file}"
     )
     debug(cmd)
+    # Test success
     res = await invoke(cmd)
     assert res.retcode == 0
 
@@ -174,8 +192,11 @@ async def test_workflow_add_task(
     assert workflow_task["args"] == ARGS
     assert workflow_task["meta"] == META
 
-    # Add a WorkflowTask with the --batch option
-    cmd = f"--batch workflow add-task {project_id} {wf.id} {t.id} --order 1"
+    # Add a WorkflowTask by Task.name with the --batch option
+    cmd = (
+        f"--batch workflow add-task {project_id} {wf.id} "
+        f"--task-name {t.name} --order 1"
+    )
     debug(cmd)
     res = await invoke(cmd)
     assert res.retcode == 0
@@ -212,7 +233,7 @@ async def test_workflow_add_task_by_name(
     task = await task_factory()
     debug(task)
 
-    cmd = f"workflow add-task {project_id} {wf.id} {task.name}"
+    cmd = f"workflow add-task {project_id} {wf.id} --task-name {task.name}"
     debug(cmd)
     res = await invoke(cmd)
     assert res.retcode == 0
@@ -221,7 +242,9 @@ async def test_workflow_add_task_by_name(
 
     # Fail when adding task via a wrong name
     with pytest.raises(SystemExit):
-        cmd = f"workflow add-task {project_id} {wf.id} INVALID_NAME"
+        cmd = (
+            f"workflow add-task {project_id} {wf.id} --task-name INVALID_NAME"
+        )
         debug(cmd)
         res = await invoke(cmd)
 
@@ -262,7 +285,7 @@ async def test_task_cache_with_non_unique_names(
     # Verify that adding tasks to a worfklow by name (as opposed to "by id")
     # fails because of missing cache file
     wf = await workflow_factory(project_id=project_id)
-    cmd = f"workflow add-task {project_id} {wf.id} {task1.name}"
+    cmd = f"workflow add-task {project_id} {wf.id} --task-name {task1.name}"
     debug(cmd)
     with pytest.raises(FileNotFoundError):
         res = await invoke(cmd)
@@ -282,7 +305,7 @@ async def test_workflow_rm_task(
     t = await task_factory()
 
     # Add task to workflow, twice
-    cmd = f"workflow add-task {project_id} {wf.id} {t.id}"
+    cmd = f"workflow add-task {project_id} {wf.id} --task-id {t.id}"
     res = await invoke(cmd)
     assert res.retcode == 0
     res = await invoke(cmd)
@@ -318,7 +341,7 @@ async def test_workflow_edit_task(
     t = await task_factory()
 
     # Create task, without overriding arguments
-    cmd = f"workflow add-task {project_id} {wf.id} {t.id}"
+    cmd = f"workflow add-task {project_id} {wf.id} --task-id {t.id}"
     res = await invoke(cmd)
     assert res.retcode == 0
 
@@ -426,7 +449,9 @@ async def test_workflow_apply(
     debug(workflow)
     assert res.retcode == 0
     TASK_ID = 1
-    res = await invoke(f"workflow add-task {prj_id} {workflow_id} {TASK_ID}")
+    res = await invoke(
+        f"workflow add-task {prj_id} {workflow_id} --task-id {TASK_ID}"
+    )
     workflow_task = res.data
     debug(workflow_task)
     assert res.retcode == 0
@@ -471,7 +496,7 @@ async def test_workflow_apply(
     with open(args_file, "w") as f:
         json.dump({"raise_error": True}, f)
     res = await invoke(
-        f"workflow add-task {prj_id} {workflow_id} {TASK_ID}"
+        f"workflow add-task {prj_id} {workflow_id} --task-id {TASK_ID}"
         f" --args-file {args_file}"
     )
     assert res.retcode == 0