Repository: ResearchObject/ro-crate-py

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.

Discussion:
sphuber: I ran into this problem and saw there was an open issue. I tried to find a solution first with keeping the place of the option as it was, but I think this is not possible with the current functionality of `click`.

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)