Code review.

src/orderly/cli/__init__.py

@@ -1,5 +1,6 @@
 import sys
 from collections import Counter
+from urllib.parse import urlparse
 
 import click
 
@@ -24,13 +25,16 @@
 @click.group(cls=PropagatingGroup)
 @with_root(propagate=True)
 def cli():
+    """Run 'orderly <command> --help' to see the options available for each subcommand."""
     pass
 
 
 @cli.command()
 @click.argument("path")
 @click.option(
-    "--archive", help="Path to the archive in which packets are stored."
+    "--archive",
+    help="Path to the archive in which packets will be stored.",
+    type=click.Path(),
 )
 @click.option(
     "--use-file-store",
@@ -47,7 +51,7 @@ def cli():
     ),
 )
 def init(path, archive, use_file_store, require_complete_tree):
-    """Initialize at new orderly repository in PATH."""
+    """Initialize at new orderly repository at the specified path."""
     kwargs = {}
     if archive is not None:
         kwargs["path_archive"] = archive
@@ -75,6 +79,7 @@ def init(path, archive, use_file_store, require_complete_tree):
     type=(str, str),
     multiple=True,
     help="Pass a string parameter to the report",
+    metavar="NAME VALUE",
 )
 @click.option(
     "-n",
@@ -83,6 +88,7 @@ def init(path, archive, use_file_store, require_complete_tree):
     type=(str, NumericalParamType()),
     multiple=True,
     help="Pass a numerical parameter to the report",
+    metavar="NAME VALUE",
 )
 @click.option(
     "-b",
@@ -91,6 +97,7 @@ def init(path, archive, use_file_store, require_complete_tree):
     type=(str, bool),
     multiple=True,
     help="Pass a boolean parameter to the report",
+    metavar="NAME VALUE",
 )
 @with_search_options
 @with_root
@@ -152,7 +159,11 @@ def search(query, root, search_options):
 @cli.group()
 @with_root(propagate=True)
 def location():
-    """Manage remote locations."""
+    """
+    Manage remote locations.
+
+    Run 'orderly location <command> --help' to see the options available for each subcommand.
+    """
     pass
 
 
@@ -184,21 +195,30 @@ def location_remove(root, name):
 
 @location.command("add")
 @click.argument("name")
-@click.argument("type", type=click.Choice(["path", "ssh"]), metavar="TYPE")
-@click.argument("location")
+@click.argument("location", metavar="(PATH | URL)")
 @with_root
-def location_add(root, name, type, location):
+def location_add(root, name, location):
     """
     Add a new remote location.
 
-    Allowed values for TYPE are "path" and "ssh".
-
-    For a location of type "path", the LOCATION argument is the path of another
-    orderly repository. For an "ssh" location, the LOCATION argument should be
-    the url to a remote server, of the form `user@hostname:path` or
-    `ssh://user@hostname:port/path`.
+    The location may be specified as a path, for outpack repositories that are
+    directly accesible through the file system, or as URLs. Currently only SSH
+    URLs are supported.
     """
-    if type == "path":
+    # On windows it could be slightly ambiguous whether "C://foo" is a URL or a
+    # file path, and we always interpret that as a URL. Users can easily
+    # disambiguate by using either backslashes or using a single forward slash.
+    if "://" not in location:
         outpack_location_add(name, "path", {"path": location}, root=root)
-    elif type == "ssh":
-        outpack_location_add(name, "ssh", {"url": location}, root=root)
+    else:
+        parts = urlparse(location)
+
+        if parts.scheme in {"ssh"}:
+            outpack_location_add(
+                name, parts.scheme, {"url": location}, root=root
+            )
+        else:
+            click.echo(
+                f"Unsupported location protocol: '{parts.scheme}'", err=True
+            )
+            sys.exit(1)

src/orderly/cli/options.py

@@ -116,6 +116,7 @@ def with_search_options(f):
             " specified multiple times to allow multiple locations. If none are"
             " specified, all configured locations are searched."
         ),
+        metavar="NAME",
     )
     @click.pass_context
     @functools.wraps(f)

tests/orderly/test_cli.py

@@ -4,7 +4,7 @@
 from orderly.cli import cli
 from pytest_unordered import unordered
 
-from outpack.config import read_config
+from outpack.config import Location, read_config
 from outpack.location import outpack_location_add_path
 from outpack.root import OutpackRoot
 
@@ -189,6 +189,33 @@ def test_search_options(tmp_path):
     assert result.stderr.strip() == "No packets matching the query were found"
 
 
+def test_can_add_locations(tmp_path):
+    root = helpers.create_temporary_roots(tmp_path)
+
+    invoke(
+        "location",
+        "add",
+        "foo",
+        root["src"],
+        *("--root", root["dst"]),
+    )
+
+    invoke(
+        "location",
+        "add",
+        "bar",
+        "ssh://127.0.0.1/foo",
+        *("--root", root["dst"]),
+    )
+
+    config = read_config(root["dst"].path)
+    assert config.location == {
+        "local": Location("local", "local", None),
+        "foo": Location("foo", "path", {"path": str(root["src"].path)}),
+        "bar": Location("bar", "ssh", {"url": "ssh://127.0.0.1/foo"}),
+    }
+
+
 def test_can_manage_locations(tmp_path):
     root = helpers.create_temporary_roots(tmp_path)
 
@@ -199,7 +226,6 @@ def test_can_manage_locations(tmp_path):
         "location",
         "add",
         "foo",
-        "path",
         root["src"],
         *("--root", root["dst"]),
     )
@@ -229,19 +255,18 @@ def test_can_manage_locations(tmp_path):
     assert result.stdout.splitlines() == ["local"]
 
 
-def test_cannot_add_location_with_unknown_type(tmp_path):
-    root = helpers.create_temporary_roots(tmp_path)
+def test_cannot_add_location_with_unknown_protocol(tmp_path):
+    root = helpers.create_temporary_root(tmp_path)
     result = invoke(
         "location",
         "add",
         "origin",
-        "blabla",
-        root["src"],
+        "myproto://example.com",
         *("--root", root),
-        expected_exit_code=2,
+        expected_exit_code=1,
     )
     assert re.search(
-        "^Error: Invalid value for 'TYPE': 'blabla' is not one of .*",
+        "^Unsupported location protocol: 'myproto'$",
         result.stderr,
         re.MULTILINE,
     )