CLI: do not except when --help is passed to sub command

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.

rocrate/cli.py

@@ -54,58 +54,58 @@ def convert(self, value, param, ctx):
 
 
 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 +115,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 +131,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/test_cli.py

@@ -14,17 +14,67 @@
 # 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
 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 +83,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 +108,11 @@ 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)
+    print(result.output)
+    print(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 +127,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 +156,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,21 +206,20 @@ 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]
+        cli, ["add", "test-instance", cli_suite_id, "http://example.com", "-c", str(crate_dir), "-r", "jobs", "-i", cli_instance_id]
     )
     assert result.exit_code == 0
     assert result.output.strip() == instance_id
@@ -180,18 +231,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)