Repository: ResearchObject/ro-crate-py (python)

Issue:
CLI can generate an error when command help is requested
In a non-ro-crate dir:

```console
$ rocrate add workflow --help
Traceback (most recent call last):
  File "/home/simleo/git/ro-crate-py/venv/bin/rocrate", line 11, in <module>
    load_entry_point('rocrate==0.5.0', 'console_scripts', 'rocrate')()
  File "/home/simleo/git/ro-crate-py/venv/lib/python3.6/site-packages/click/core.py", line 1128, in __call__
    return self.main(*args, **kwargs)
  File "/home/simleo/git/ro-crate-py/venv/lib/python3.6/site-packages/click/core.py", line 1053, in main
    rv = self.invoke(ctx)
  File "/home/simleo/git/ro-crate-py/venv/lib/python3.6/site-packages/click/core.py", line 1659, in invoke
    return _process_result(sub_ctx.command.invoke(sub_ctx))
  File "/home/simleo/git/ro-crate-py/venv/lib/python3.6/site-packages/click/core.py", line 1656, in invoke
    super().invoke(ctx)
  File "/home/simleo/git/ro-crate-py/venv/lib/python3.6/site-packages/click/core.py", line 1395, in invoke
    return ctx.invoke(self.callback, **ctx.params)
  File "/home/simleo/git/ro-crate-py/venv/lib/python3.6/site-packages/click/core.py", line 754, in invoke
    return __callback(*args, **kwargs)
  File "/home/simleo/git/ro-crate-py/venv/lib/python3.6/site-packages/click/decorators.py", line 38, in new_func
    return f(get_current_context().obj, *args, **kwargs)
  File "/home/simleo/git/ro-crate-py/venv/lib/python3.6/site-packages/rocrate/cli.py", line 58, in add
    state.crate = ROCrate(state.crate_dir, init=False, gen_preview=False)
  File "/home/simleo/git/ro-crate-py/venv/lib/python3.6/site-packages/rocrate/rocrate.py", line 90, in __init__
    source = self.__read(source, gen_preview=gen_preview)
  File "/home/simleo/git/ro-crate-py/venv/lib/python3.6/site-packages/rocrate/rocrate.py", line 129, in __read
    raise ValueError(f"Not a valid RO-Crate: missing {Metadata.BASENAME}")
ValueError: Not a valid RO-Crate: missing ro-crate-metadata.json
```

This should not happen, since the user is not actually trying to do anything ro-crate specific.

Pull request:
CLI: do not except when `--help` is passed to sub command
Fixes #97 

The `rocrate add workflow --help` call would except because the `add`
command group would attempt to instantiate a research object crate. But
since no existing crate was defined through the `-c` option, the
instantiation of the crate would except.

There is no straightforward way in `click` to prevent the execution of
group command code when `--help` is passed. Instead, we move the
addition of the `-c/--crate-dir` option to those commands that actually
need it. Code duplication is prevented by defining the option once,
assigning it to `OPTION_CRATE_PATH` and using that decorator for each
command that requires the option.

This is a breaking change for the interface as the crate path should now
no longer be specified directly after the main command but rather after
the actual leaf command. So:

    rocrate -c some/ro-crate add workflow

Now becomes

    rocrate add workflow -c some/ro-crate

The advantage of this is that it doesn't enforce this option on all
commands, even those that don't need it and it is probably more
intuitive for the user to put the option closer to all other options.

Patch:
diff --git a/rocrate/cli.py b/rocrate/cli.py
index a74ec90..38a20d0 100644
--- a/rocrate/cli.py
+++ b/rocrate/cli.py
@@ -54,58 +54,57 @@ class CSVParamType(click.ParamType):
 
 
 CSV = CSVParamType()
+OPTION_CRATE_PATH = click.option('-c', '--crate-dir', type=click.Path(), default=os.getcwd)
 
 
 @click.group()
-@click.option('-c', '--crate-dir', type=click.Path())
-@click.pass_context
-def cli(ctx, crate_dir):
-    ctx.obj = state = State()
-    state.crate_dir = os.getcwd() if not crate_dir else os.path.abspath(crate_dir)
+def cli():
+    pass
 
 
 @cli.command()
 @click.option('--gen-preview', is_flag=True)
 @click.option('-e', '--exclude', type=CSV)
-@click.pass_obj
-def init(state, gen_preview, exclude):
-    crate = ROCrate(state.crate_dir, init=True, gen_preview=gen_preview, exclude=exclude)
-    crate.metadata.write(state.crate_dir)
+@OPTION_CRATE_PATH
+def init(crate_dir, gen_preview, exclude):
+    crate = ROCrate(crate_dir, init=True, gen_preview=gen_preview, exclude=exclude)
+    crate.metadata.write(crate_dir)
     if crate.preview:
-        crate.preview.write(state.crate_dir)
+        crate.preview.write(crate_dir)
 
 
 @cli.group()
-@click.pass_obj
-def add(state):
-    state.crate = ROCrate(state.crate_dir, init=False, gen_preview=False)
+def add():
+    pass
 
 
 @add.command()
 @click.argument('path', type=click.Path(exists=True))
 @click.option('-l', '--language', type=click.Choice(LANG_CHOICES), default="cwl")
-@click.pass_obj
-def workflow(state, path, language):
+@OPTION_CRATE_PATH
+def workflow(crate_dir, path, language):
+    crate = ROCrate(crate_dir, init=False, gen_preview=False)
     source = Path(path).resolve(strict=True)
     try:
-        dest_path = source.relative_to(state.crate_dir)
+        dest_path = source.relative_to(crate_dir)
     except ValueError:
         # For now, only support marking an existing file as a workflow
-        raise ValueError(f"{source} is not in the crate dir {state.crate_dir}")
+        raise ValueError(f"{source} is not in the crate dir {crate_dir}")
     # TODO: add command options for main and gen_cwl
-    state.crate.add_workflow(source, dest_path, main=True, lang=language, gen_cwl=False)
-    state.crate.metadata.write(state.crate_dir)
+    crate.add_workflow(source, dest_path, main=True, lang=language, gen_cwl=False)
+    crate.metadata.write(crate_dir)
 
 
 @add.command(name="test-suite")
 @click.option('-i', '--identifier')
 @click.option('-n', '--name')
 @click.option('-m', '--main-entity')
-@click.pass_obj
-def suite(state, identifier, name, main_entity):
-    suite_ = state.crate.add_test_suite(identifier=add_hash(identifier), name=name, main_entity=main_entity)
-    state.crate.metadata.write(state.crate_dir)
-    print(suite_.id)
+@OPTION_CRATE_PATH
+def suite(crate_dir, identifier, name, main_entity):
+    crate = ROCrate(crate_dir, init=False, gen_preview=False)
+    suite = crate.add_test_suite(identifier=add_hash(identifier), name=name, main_entity=main_entity)
+    crate.metadata.write(crate_dir)
+    print(suite.id)
 
 
 @add.command(name="test-instance")
@@ -115,13 +114,14 @@ def suite(state, identifier, name, main_entity):
 @click.option('-s', '--service', type=click.Choice(SERVICE_CHOICES), default="jenkins")
 @click.option('-i', '--identifier')
 @click.option('-n', '--name')
-@click.pass_obj
-def instance(state, suite, url, resource, service, identifier, name):
-    instance_ = state.crate.add_test_instance(
+@OPTION_CRATE_PATH
+def instance(crate_dir, suite, url, resource, service, identifier, name):
+    crate = ROCrate(crate_dir, init=False, gen_preview=False)
+    instance_ = crate.add_test_instance(
         add_hash(suite), url, resource=resource, service=service,
         identifier=add_hash(identifier), name=name
     )
-    state.crate.metadata.write(state.crate_dir)
+    crate.metadata.write(crate_dir)
     print(instance_.id)
 
 
@@ -130,23 +130,24 @@ def instance(state, suite, url, resource, service, identifier, name):
 @click.argument('path', type=click.Path(exists=True))
 @click.option('-e', '--engine', type=click.Choice(ENGINE_CHOICES), default="planemo")
 @click.option('-v', '--engine-version')
-@click.pass_obj
-def definition(state, suite, path, engine, engine_version):
+@OPTION_CRATE_PATH
+def definition(crate_dir, suite, path, engine, engine_version):
+    crate = ROCrate(crate_dir, init=False, gen_preview=False)
     source = Path(path).resolve(strict=True)
     try:
-        dest_path = source.relative_to(state.crate_dir)
+        dest_path = source.relative_to(crate_dir)
     except ValueError:
         # For now, only support marking an existing file as a test definition
-        raise ValueError(f"{source} is not in the crate dir {state.crate_dir}")
-    state.crate.add_test_definition(suite, source=source, dest_path=dest_path, engine=engine, engine_version=engine_version)
-    state.crate.metadata.write(state.crate_dir)
+        raise ValueError(f"{source} is not in the crate dir {crate_dir}")
+    crate.add_test_definition(suite, source=source, dest_path=dest_path, engine=engine, engine_version=engine_version)
+    crate.metadata.write(crate_dir)
 
 
 @cli.command()
 @click.argument('dst', type=click.Path(writable=True))
-@click.pass_obj
-def write_zip(state, dst):
-    crate = ROCrate(state.crate_dir, init=False, gen_preview=False)
+@OPTION_CRATE_PATH
+def write_zip(crate_dir, dst):
+    crate = ROCrate(crate_dir, init=False, gen_preview=False)
     crate.write_zip(dst)
 
 


Test patch:
diff --git a/test/test_cli.py b/test/test_cli.py
index adcda3b..0d9af09 100644
--- a/test/test_cli.py
+++ b/test/test_cli.py
@@ -14,10 +14,11 @@
 # WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
 # See the License for the specific language governing permissions and
 # limitations under the License.
-
 import json
-import pytest
+
+import click
 from click.testing import CliRunner
+import pytest
 
 from rocrate.cli import cli
 from rocrate.model.file import File
@@ -25,6 +26,66 @@ from rocrate.model.metadata import TESTING_EXTRA_TERMS
 from rocrate.rocrate import ROCrate
 
 
+def get_command_paths(command):
+    """\
+    Return a list of full command paths for all leaf commands that are part of
+    the given root command.
+
+    For example, for a root command that has two subcommands ``command_a`` and
+    ``command_b`` where ``command_b`` in turn has two subcommands, the
+    returned list will have the form::
+
+        [
+            ['command_a'],
+            ['command_b', 'subcommand_a'],
+            ['command_b', 'subcommand_b'],
+        ]
+
+    :param command: The root command.
+    :return: A list of lists, where each element is the full command path to a
+      leaf command.
+    """
+
+    def resolve_commands(command, command_path, commands):
+        if isinstance(command, click.MultiCommand):
+            for subcommand in command.commands.values():
+                command_subpath = command_path + [subcommand.name]
+                resolve_commands(subcommand, command_subpath, commands)
+        else:
+            commands.append(command_path)
+
+    commands = []
+    resolve_commands(command, [], commands)
+
+    return commands
+
+
+@pytest.mark.parametrize('command_path', get_command_paths(cli))
+def test_cli_help(command_path):
+    """\
+    Test that invoking any CLI command with ``--help`` prints the help string
+    and exits normally.
+
+    This is a regression test for:
+    https://github.com/ResearchObject/ro-crate-py/issues/97
+
+    Note that we cannot simply invoke the actual leaf :class:`click.Command`
+    that we are testing, because the test runner follows a different path then
+    when actually invoking the command from the command line. This means that
+    any code that is in the groups that the command is part of, won't be
+    executed. This in turn means that a command could actually be broken when
+    invoked from the command line but would not be detected by the test. The
+    workaround is to invoke the full command path. For example when testing
+    ``add workflow --help``, we cannot simply invoke ``workflow`` with
+    ``['--help']`` as argument but we need to invoke the base command with
+    ``['add', 'workflow', '--help']``.
+    """
+    runner = CliRunner()
+    result = runner.invoke(cli, command_path + ['--help'])
+    assert result.exit_code == 0, result.output
+    assert 'Usage:' in result.output
+
+
 @pytest.mark.parametrize("gen_preview,cwd", [(False, False), (False, True), (True, False), (True, True)])
 def test_cli_init(test_data_dir, helpers, monkeypatch, cwd, gen_preview):
     crate_dir = test_data_dir / "ro-crate-galaxy-sortchangecase"
@@ -33,12 +94,11 @@ def test_cli_init(test_data_dir, helpers, monkeypatch, cwd, gen_preview):
     metadata_path.unlink()
 
     runner = CliRunner()
-    args = []
+    args = ["init"]
     if cwd:
         monkeypatch.chdir(str(crate_dir))
     else:
         args.extend(["-c", str(crate_dir)])
-    args.append("init")
     if gen_preview:
         args.append("--gen-preview")
     result = runner.invoke(cli, args)
@@ -59,8 +119,9 @@ def test_cli_init_exclude(test_data_dir, helpers):
     (crate_dir / helpers.METADATA_FILE_NAME).unlink()
     exclude = "test,README.md"
     runner = CliRunner()
-    args = ["-c", str(crate_dir), "init", "-e", exclude]
-    assert runner.invoke(cli, args).exit_code == 0
+    args = ["init", "-c", str(crate_dir), "-e", exclude]
+    result = runner.invoke(cli, args)
+    assert result.exit_code == 0
     crate = ROCrate(crate_dir)
     for p in "LICENSE", "sort-and-change-case.ga":
         assert isinstance(crate.dereference(p), File)
@@ -75,20 +136,20 @@ def test_cli_add_workflow(test_data_dir, helpers, monkeypatch, cwd):
     # init
     crate_dir = test_data_dir / "ro-crate-galaxy-sortchangecase"
     runner = CliRunner()
-    assert runner.invoke(cli, ["-c", str(crate_dir), "init"]).exit_code == 0
+    assert runner.invoke(cli, ["init", "-c", str(crate_dir)]).exit_code == 0
     json_entities = helpers.read_json_entities(crate_dir)
     assert "sort-and-change-case.ga" in json_entities
     assert json_entities["sort-and-change-case.ga"]["@type"] == "File"
     # add
     wf_path = crate_dir / "sort-and-change-case.ga"
-    args = []
+    args = ["add", "workflow"]
     if cwd:
         monkeypatch.chdir(str(crate_dir))
         wf_path = wf_path.relative_to(crate_dir)
     else:
         args.extend(["-c", str(crate_dir)])
     for lang in "cwl", "galaxy":
-        extra_args = ["add", "workflow", "-l", lang, str(wf_path)]
+        extra_args = ["-l", lang, str(wf_path)]
         result = runner.invoke(cli, args + extra_args)
         assert result.exit_code == 0
         json_entities = helpers.read_json_entities(crate_dir)
@@ -104,35 +165,35 @@ def test_cli_add_test_metadata(test_data_dir, helpers, monkeypatch, cwd):
     # init
     crate_dir = test_data_dir / "ro-crate-galaxy-sortchangecase"
     runner = CliRunner()
-    assert runner.invoke(cli, ["-c", str(crate_dir), "init"]).exit_code == 0
+    assert runner.invoke(cli, ["init", "-c", str(crate_dir)]).exit_code == 0
     json_entities = helpers.read_json_entities(crate_dir)
     def_id = "test/test1/sort-and-change-case-test.yml"
     assert def_id in json_entities
     assert json_entities[def_id]["@type"] == "File"
     # add workflow
     wf_path = crate_dir / "sort-and-change-case.ga"
-    assert runner.invoke(cli, ["-c", str(crate_dir), "add", "workflow", "-l", "galaxy", str(wf_path)]).exit_code == 0
+    assert runner.invoke(cli, ["add", "workflow", "-c", str(crate_dir), "-l", "galaxy", str(wf_path)]).exit_code == 0
     # add test suite
-    result = runner.invoke(cli, ["-c", str(crate_dir), "add", "test-suite"])
+    result = runner.invoke(cli, ["add", "test-suite", "-c", str(crate_dir)])
     assert result.exit_code == 0
     suite_id = result.output.strip()
     json_entities = helpers.read_json_entities(crate_dir)
     assert suite_id in json_entities
     # add test instance
-    result = runner.invoke(cli, ["-c", str(crate_dir), "add", "test-instance", suite_id, "http://example.com", "-r", "jobs"])
+    result = runner.invoke(cli, ["add", "test-instance", "-c", str(crate_dir), suite_id, "http://example.com", "-r", "jobs"])
     assert result.exit_code == 0
     instance_id = result.output.strip()
     json_entities = helpers.read_json_entities(crate_dir)
     assert instance_id in json_entities
     # add test definition
     def_path = crate_dir / def_id
-    args = []
+    args = ["add", "test-definition"]
     if cwd:
         monkeypatch.chdir(str(crate_dir))
         def_path = def_path.relative_to(crate_dir)
     else:
         args.extend(["-c", str(crate_dir)])
-    extra_args = ["add", "test-definition", "-e", "planemo", "-v", ">=0.70", suite_id, str(def_path)]
+    extra_args = ["-e", "planemo", "-v", ">=0.70", suite_id, str(def_path)]
     result = runner.invoke(cli, args + extra_args)
     assert result.exit_code == 0
     json_entities = helpers.read_json_entities(crate_dir)
@@ -154,22 +215,22 @@ def test_cli_add_test_metadata(test_data_dir, helpers, monkeypatch, cwd):
 def test_cli_add_test_metadata_explicit_ids(test_data_dir, helpers, monkeypatch, hash_):
     crate_dir = test_data_dir / "ro-crate-galaxy-sortchangecase"
     runner = CliRunner()
-    assert runner.invoke(cli, ["-c", str(crate_dir), "init"]).exit_code == 0
+    assert runner.invoke(cli, ["init", "-c", str(crate_dir)]).exit_code == 0
     wf_path = crate_dir / "sort-and-change-case.ga"
-    assert runner.invoke(cli, ["-c", str(crate_dir), "add", "workflow", "-l", "galaxy", str(wf_path)]).exit_code == 0
+    assert runner.invoke(cli, ["add", "workflow", "-c", str(crate_dir), "-l", "galaxy", str(wf_path)]).exit_code == 0
     suite_id = "#foo"
     cli_suite_id = suite_id if hash_ else suite_id[1:]
-    result = runner.invoke(cli, ["-c", str(crate_dir), "add", "test-suite", "-i", cli_suite_id])
+    result = runner.invoke(cli, ["add", "test-suite", "-c", str(crate_dir), "-i", cli_suite_id])
     assert result.exit_code == 0
     assert result.output.strip() == suite_id
     json_entities = helpers.read_json_entities(crate_dir)
     assert suite_id in json_entities
     instance_id = "#bar"
     cli_instance_id = instance_id if hash_ else instance_id[1:]
-    result = runner.invoke(
-        cli, ["-c", str(crate_dir), "add", "test-instance", cli_suite_id,
-              "http://example.com", "-r", "jobs", "-i", cli_instance_id]
-    )
+    args = [
+        "add", "test-instance", cli_suite_id, "http://example.com", "-c", str(crate_dir), "-r", "jobs", "-i", cli_instance_id
+    ]
+    result = runner.invoke(cli, args)
     assert result.exit_code == 0
     assert result.output.strip() == instance_id
     json_entities = helpers.read_json_entities(crate_dir)
@@ -180,18 +241,17 @@ def test_cli_add_test_metadata_explicit_ids(test_data_dir, helpers, monkeypatch,
 def test_cli_write_zip(test_data_dir, monkeypatch, cwd):
     crate_dir = test_data_dir / "ro-crate-galaxy-sortchangecase"
     runner = CliRunner()
-    assert runner.invoke(cli, ["-c", str(crate_dir), "init"]).exit_code == 0
+    assert runner.invoke(cli, ["init", "-c", str(crate_dir)]).exit_code == 0
     wf_path = crate_dir / "sort-and-change-case.ga"
-    args = ["-c", str(crate_dir), "add", "workflow", str(wf_path)]
+    args = ["add", "workflow", str(wf_path), "-c", str(crate_dir)]
     assert runner.invoke(cli, args).exit_code == 0
 
     output_zip_path = test_data_dir / "test-zip-archive.zip"
-    args = []
+    args = ["write-zip"]
     if cwd:
         monkeypatch.chdir(str(crate_dir))
     else:
         args.extend(["-c", str(crate_dir)])
-    args.append("write-zip")
     args.append(str(output_zip_path))
 
     result = runner.invoke(cli, args)