fix the types in --copy and --entrypoint (#500)

* fix the types in --copy and --entrypoint

Both of these options use the class OptionEatAll. By default, this class
will output a string type, but that would result in a string that looked
like a tuple. For example, it is possible to use `ast.literal_eval(value)`
on the value to convert it back to a tuple. To fix this, this commit
sets the type for --copy and --entrypoint to `tuple` and the subsequent
code expects the values for --copy and --entrypoint to be tuples.

This commit also adds a type hint to an OptionsEatAll internal to help
with debugging.

* expand issue 498 tests for --copy and --entrypoint

* change assertions to raise click.ClickException

This formats the error messages better (and with less clutter). This
commit also adds a note to contact the devs in the case of errors that
users cannot fix (errors in the implementation...).

* raise an error if OptionEatAll is used as a str type

The fundamental issue leading to #498 was that the values in
OptionEatAll were being coerced to a string type if no type was
specified in OptionEatAll. This made a tuple `('foo',)` into a string
`'("foo",)'`. This commit raises an error if OptionEatAll is used as a
string. Indeed, the 'eat-all' nature of this option suggests that there
are multiple options, which suggests the type should be a list or tuple.

This commit also adds a test for this.

neurodocker/cli/generate.py

@@ -122,11 +122,16 @@ def __init__(self, *args, **kwargs):
         nargs = kwargs.pop("nargs", -1)
         assert nargs == -1, "nargs, if set, must be -1 not {}".format(nargs)
         super(OptionEatAll, self).__init__(*args, **kwargs)
+        if self.type is click.STRING:
+            raise ValueError(
+                "in the current implementation of OptionEatAll, `type` cannot be a"
+                " string."
+            )
         self._previous_parser_process = None
         self._eat_all_parser = None
 
     def add_to_parser(self, parser, ctx):
-        def parser_process(value, state):
+        def parser_process(value, state: click.parser.ParsingState):
             # method to hook to the parser.process
             done = False
             value = [value]
@@ -201,6 +206,7 @@ def _get_common_renderer_params() -> ty.List[click.Parameter]:
         OptionEatAll(
             ["--copy"],
             multiple=True,
+            type=tuple,
             help=(
                 "Copy files into the container. Provide at least two paths."
                 " The last path is always the destination path in the container."
@@ -215,6 +221,7 @@ def _get_common_renderer_params() -> ty.List[click.Parameter]:
         OptionEatAll(
             ["--entrypoint"],
             multiple=True,
+            type=tuple,
             help="Set entrypoint of the container",
         ),
         OptionEatAll(
@@ -317,12 +324,16 @@ def _get_instruction_for_param(
         d = {"name": param.name, "kwds": {"base_image": value}}
     # arg
     elif param.name == "arg":
-        assert len(value) == 2, "expected key=value pair for --arg"
+        if len(value) != 2:
+            raise click.ClickException("expected key=value pair for --arg")
         k, v = value
         d = {"name": param.name, "kwds": {"key": k, "value": v}}
     # copy
     elif param.name == "copy":
-        assert len(value) > 1, "expected at least two values for --copy"
+        if not isinstance(value, tuple):
+            raise ValueError("expected this value to be a tuple (contact developers)")
+        if len(value) < 2:
+            raise click.ClickException("expected at least two values for --copy")
         source, destination = list(value[:-1]), value[-1]
         d = {"name": param.name, "kwds": {"source": source, "destination": destination}}
     # env
@@ -331,10 +342,9 @@ def _get_instruction_for_param(
         d = {"name": param.name, "kwds": {**value}}
     # entrypoint
     elif param.name == "entrypoint":
-        if isinstance(value, str):
-            value = [value]
-        else:
-            value = list(value)
+        if not isinstance(value, tuple):
+            raise ValueError("expected this value to be a tuple (contact developers)")
+        value = list(value)  # convert from tuple to list
         d = {"name": param.name, "kwds": {"args": value}}
     # install
     elif param.name == "install":

neurodocker/cli/tests/test_cli.py

@@ -6,6 +6,7 @@
 import pytest
 
 from neurodocker.cli.cli import generate
+from neurodocker.cli.generate import OptionEatAll
 
 _cmds = ["docker", "singularity"]
 
@@ -42,8 +43,7 @@ def test_minimal_args(cmd: str, pkg_manager: str):
     assert result.exit_code == 0, result.output
 
 
-@pytest.mark.xfail(reason="https://github.com/ReproNim/neurodocker/issues/498")
-def test_copy():
+def test_copy_issue_498():
     runner = CliRunner()
     result = runner.invoke(
         generate,
@@ -59,7 +59,51 @@ def test_copy():
             "file2",
         ],
     )
-    assert "file1" in (result.output)
+    assert "file1" in result.output
+    assert '\nCOPY ["file1", \\\n      "file2"]\n' in result.output
+
+    # Fail if given fewer than two inputs to --copy.
+    result = runner.invoke(
+        generate,
+        [
+            "docker",
+            "--pkg-manager",
+            "apt",
+            "--base-image",
+            "debian",
+            # copy
+            "--copy",
+            "file1",
+        ],
+    )
+    assert "Error: expected at least two values for --copy" in result.output
+    assert result.exit_code != 0
+
+
+# Issue #498 references --copy but the same broken behavior is seen in --entrypoint.
+def test_entrypoint_issue_498():
+    runner = CliRunner()
+    result = runner.invoke(
+        generate,
+        [
+            "docker",
+            "--pkg-manager",
+            "apt",
+            "--base-image",
+            "debian",
+            "--entrypoint",
+            "printf",
+            "this",
+            "that",
+        ],
+    )
+    assert '\nENTRYPOINT ["printf", "this", "that"]\n' in result.output
+
+
+def test_optioneatall_type_issue_498():
+    with pytest.raises(ValueError):
+        OptionEatAll(["--foo"], type=str)
+    OptionEatAll(["--foo"], type=tuple)
 
 
 @pytest.mark.parametrize("cmd", _cmds)