Positional arguments should not lead to removal of short form of keyw…

…ord arguments (fixes #115) (#181)

* refactor: keep it simple
* fix: positional arguments should not lead to removal of short form of keyword arguments (#115)
* docs: update changelog

CHANGES

@@ -25,6 +25,11 @@ Enhancements:
 - Can control exit status (see Backwards Incompatible Changes above) when raising
   ``CommandError`` using the ``code`` keyword arg.
 
+Bugs fixed:
+
+-  Positional arguments should not lead to removal of short form of keyword
+   arguments. (#115)
+
 Other changes:
 
 - Avoid depending on iocapture by using pytest's built-in feature (#177)

src/argh/assembling.py

@@ -67,9 +67,14 @@ def _get_args_from_signature(function):
 
     # define the list of conflicting option strings
     # (short forms, i.e. single-character ones)
-    chars = [a[0] for a in spec.args + kwonly]
-    char_counts = dict((char, chars.count(char)) for char in set(chars))
-    conflicting_opts = tuple(char for char in char_counts if 1 < char_counts[char])
+    named_args = set(list(defaults) + kwonly)
+    named_arg_chars = [a[0] for a in named_args]
+    named_arg_char_counts = dict(
+        (char, named_arg_chars.count(char)) for char in set(named_arg_chars)
+    )
+    conflicting_opts = tuple(
+        char for char in named_arg_char_counts if 1 < named_arg_char_counts[char]
+    )
 
     for name in spec.args + kwonly:
         flags = []  # name_or_flags
@@ -289,12 +294,11 @@ def set_default_command(parser, function):
             action = parser.add_argument(*dest_or_opt_strings, **draft)
             if COMPLETION_ENABLED and completer:
                 action.completer = completer
-        except Exception as e:
-            raise type(e)(
-                "{func}: cannot add arg {args}: {msg}".format(
-                    args="/".join(dest_or_opt_strings), func=function.__name__, msg=e
-                )
-            )
+        except Exception as exc:
+            err_args = "/".join(dest_or_opt_strings)
+            raise AssemblingError(
+                f"{function.__name__}: cannot add {err_args}: {exc}"
+            ) from exc
 
     if function.__doc__ and not parser.description:
         parser.description = function.__doc__

tests/test_assembling.py

@@ -2,7 +2,7 @@
 Unit Tests For Assembling Phase
 ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
 """
-from unittest import mock
+from unittest.mock import MagicMock, call, patch
 
 import pytest
 
@@ -66,22 +66,96 @@ def func():
 
     parser = argh.ArghParser()
 
-    parser.add_argument = mock.MagicMock()
-    parser.set_defaults = mock.MagicMock()
+    parser.add_argument = MagicMock()
+    parser.set_defaults = MagicMock()
 
     argh.set_default_command(parser, func)
 
     assert parser.add_argument.mock_calls == [
-        mock.call("foo", nargs="+", choices=[1, 2], help="me", type=int),
-        mock.call(
+        call("foo", nargs="+", choices=[1, 2], help="me", type=int),
+        call(
             "-b",
             "--bar",
             default=False,
             action="store_true",
             help=argh.constants.DEFAULT_ARGUMENT_TEMPLATE,
         ),
     ]
-    assert parser.set_defaults.mock_calls == [mock.call(function=func)]
+    assert parser.set_defaults.mock_calls == [call(function=func)]
+
+
+def test_set_default_command_infer_cli_arg_names_from_func_signature():
+    # TODO: split into small tests where we'd check each combo and make sure
+    # they interact as expected (e.g. pos opt arg gets the short form even if
+    # there's a pos req arg, etc.)
+    #
+    # an arg with a unique first letter per arg type + every combination
+    # of conflicting first letters per every two arg types:
+    # - positional required (i.e. without a default value)
+    # - positional optional (i.e. with a default value)
+    # - named-only required (i.e. kwonly without a default value)
+    # - named-only optional (i.e. kwonly with a default valu)
+    def func(
+        alpha_pos_req,
+        beta_pos_req,
+        alpha_pos_opt="alpha",
+        beta_pos_opt_one="beta one",
+        beta_pos_opt_two="beta two",
+        gamma_pos_opt="gamma named",
+        delta_pos_opt="delta named",
+        theta_pos_opt="theta named",
+        *args,
+        gamma_kwonly_opt="gamma kwonly",
+        delta_kwonly_req,
+        epsilon_kwonly_req_one,
+        epsilon_kwonly_req_two,
+        zeta_kwonly_opt="zeta kwonly",
+        **kwargs,
+    ):
+        return (
+            alpha_pos_req,
+            beta_pos_req,
+            alpha_pos_opt,
+            beta_pos_opt_one,
+            beta_pos_opt_two,
+            gamma_pos_opt,
+            delta_pos_opt,
+            gamma_kwonly_opt,
+            delta_kwonly_req,
+            epsilon_kwonly_req_one,
+            epsilon_kwonly_req_two,
+            zeta_kwonly_opt,
+            args,
+            kwargs,
+        )
+
+    parser = argh.ArghParser()
+
+    parser.add_argument = MagicMock()
+    parser.set_defaults = MagicMock()
+
+    argh.set_default_command(parser, func)
+
+    help_tmpl = argh.constants.DEFAULT_ARGUMENT_TEMPLATE
+    assert parser.add_argument.mock_calls == [
+        call("alpha_pos_req", help="%(default)s"),
+        call("beta_pos_req", help="%(default)s"),
+        call("-a", "--alpha-pos-opt", default="alpha", type=str, help=help_tmpl),
+        call("--beta-pos-opt-one", default="beta one", type=str, help=help_tmpl),
+        call("--beta-pos-opt-two", default="beta two", type=str, help=help_tmpl),
+        call("--gamma-pos-opt", default="gamma named", type=str, help=help_tmpl),
+        call("--delta-pos-opt", default="delta named", type=str, help=help_tmpl),
+        call("-t", "--theta-pos-opt", default="theta named", type=str, help=help_tmpl),
+        call("--gamma-kwonly-opt", default="gamma kwonly", type=str, help=help_tmpl),
+        call("--delta-kwonly-req", required=True, help=help_tmpl),
+        call("--epsilon-kwonly-req-one", required=True, help=help_tmpl),
+        call("--epsilon-kwonly-req-two", required=True, help=help_tmpl),
+        call(
+            "-z", "--zeta-kwonly-opt", default="zeta kwonly", type=str, help=help_tmpl
+        ),
+        call("args", nargs="*", help=help_tmpl),
+    ]
+    assert parser.set_defaults.mock_calls == [call(function=func)]
 
 
 def test_set_default_command_docstring():
@@ -161,9 +235,9 @@ def func():
 
     p = argh.ArghParser()
 
-    with pytest.raises(ValueError) as excinfo:
+    with pytest.raises(argh.AssemblingError) as excinfo:
         p.set_default_command(func)
-    msg = "func: cannot add arg x/--y: invalid option string"
+    msg = "func: cannot add x/--y: invalid option string"
     assert msg in str(excinfo.value)
 
 
@@ -173,14 +247,12 @@ def func(*file_paths):
 
     parser = argh.ArghParser()
 
-    parser.add_argument = mock.MagicMock()
+    parser.add_argument = MagicMock()
 
     argh.set_default_command(parser, func)
 
     assert parser.add_argument.mock_calls == [
-        mock.call(
-            "file_paths", nargs="*", help=argh.constants.DEFAULT_ARGUMENT_TEMPLATE
-        ),
+        call("file_paths", nargs="*", help=argh.constants.DEFAULT_ARGUMENT_TEMPLATE),
     ]
 
 
@@ -192,14 +264,14 @@ def func(x, **kwargs):
 
     parser = argh.ArghParser()
 
-    parser.add_argument = mock.MagicMock()
+    parser.add_argument = MagicMock()
 
     argh.set_default_command(parser, func)
 
     assert parser.add_argument.mock_calls == [
-        mock.call("x", help=argh.constants.DEFAULT_ARGUMENT_TEMPLATE),
-        mock.call("foo", help=argh.constants.DEFAULT_ARGUMENT_TEMPLATE),
-        mock.call("--bar", help=argh.constants.DEFAULT_ARGUMENT_TEMPLATE),
+        call("x", help=argh.constants.DEFAULT_ARGUMENT_TEMPLATE),
+        call("foo", help=argh.constants.DEFAULT_ARGUMENT_TEMPLATE),
+        call("--bar", help=argh.constants.DEFAULT_ARGUMENT_TEMPLATE),
     ]
 
 
@@ -222,28 +294,24 @@ def cmd(foo: "quux" = 123):  # noqa: F821
 
 def test_kwonlyargs():
     "Correctly processing required and optional keyword-only arguments"
-    ns = {}
-    exec("def cmd(*args, foo='abcd', bar):\n" "    return (args, foo, bar)", None, ns)
-    cmd = ns["cmd"]
+
+    def cmd(foo_pos, bar_pos, *args, foo_kwonly="foo_kwonly", bar_kwonly):
+        return (foo_pos, bar_pos, args, foo_kwonly, bar_kwonly)
+
     p = argh.ArghParser()
-    p.add_argument = mock.MagicMock()
+    p.add_argument = MagicMock()
     p.set_default_command(cmd)
+    help_tmpl = argh.constants.DEFAULT_ARGUMENT_TEMPLATE
     assert p.add_argument.mock_calls == [
-        mock.call(
-            "-f",
-            "--foo",
-            type=str,
-            default="abcd",
-            help=argh.constants.DEFAULT_ARGUMENT_TEMPLATE,
-        ),
-        mock.call(
-            "-b", "--bar", required=True, help=argh.constants.DEFAULT_ARGUMENT_TEMPLATE
-        ),
-        mock.call("args", nargs="*", help=argh.constants.DEFAULT_ARGUMENT_TEMPLATE),
+        call("foo_pos", help=help_tmpl),
+        call("bar_pos", help=help_tmpl),
+        call("-f", "--foo-kwonly", default="foo_kwonly", type=str, help=help_tmpl),
+        call("-b", "--bar-kwonly", required=True, help=help_tmpl),
+        call("args", nargs="*", help=help_tmpl),
     ]
 
 
-@mock.patch("argh.assembling.COMPLETION_ENABLED", True)
+@patch("argh.assembling.COMPLETION_ENABLED", True)
 def test_custom_argument_completer():
     "Issue #33: Enable custom per-argument shell completion"
 
@@ -262,7 +330,7 @@ def func(foo):
     assert p._actions[-1].completer == "STUB"
 
 
-@mock.patch("argh.assembling.COMPLETION_ENABLED", False)
+@patch("argh.assembling.COMPLETION_ENABLED", False)
 def test_custom_argument_completer_no_backend():
     "If completion backend is not available, nothing breaks"
 
@@ -281,6 +349,7 @@ def func(foo):
     assert not hasattr(p._actions[-1], "completer")
 
 
+# TODO: remove in v.0.30
 def test_set_default_command_deprecation_warnings():
     parser = argh.ArghParser()
 
@@ -301,9 +370,9 @@ def test_set_default_command_deprecation_warnings():
         argh.add_commands(parser, [], group_name="c", help="bar")
 
 
-@mock.patch("argh.assembling.add_commands")
+@patch("argh.assembling.add_commands")
 def test_add_subcommands(mock_add_commands):
-    mock_parser = mock.MagicMock()
+    mock_parser = MagicMock()
 
     def get_items():
         pass
@@ -327,7 +396,7 @@ def get_items():
     )
 
 
-@mock.patch("argh.helpers.autocomplete")
+@patch("argh.helpers.autocomplete")
 def test_arghparser_autocomplete_method(mock_autocomplete):
     p = argh.ArghParser()
     p.autocomplete()

tests/test_integration.py

@@ -731,16 +731,9 @@ def static_meth2(value):
 
 def test_kwonlyargs():
     "Correct dispatch in presence of keyword-only arguments"
-    ns = {}
-
-    exec(
-        """def cmd(*args, foo='1', bar, baz='3', **kwargs):
-                return ' '.join(args), foo, bar, baz, len(kwargs)
-         """,
-        None,
-        ns,
-    )
-    cmd = ns["cmd"]
+
+    def cmd(*args, foo="1", bar, baz="3", **kwargs):
+        return " ".join(args), foo, bar, baz, len(kwargs)
 
     p = DebugArghParser()
     p.set_default_command(cmd)