From ae94c12af9fa111f3d4722de8833daafb03e7380 Mon Sep 17 00:00:00 2001 From: Andy Mikhailenko Date: Sat, 26 Aug 2023 21:46:01 +0200 Subject: [PATCH] Positional arguments should not lead to removal of short form of keyword 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 | 5 ++ src/argh/assembling.py | 22 +++--- tests/test_assembling.py | 141 ++++++++++++++++++++++++++++---------- tests/test_integration.py | 13 +--- 4 files changed, 126 insertions(+), 55 deletions(-) diff --git a/CHANGES b/CHANGES index 703a60a..b1d7b3c 100644 --- a/CHANGES +++ b/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) diff --git a/src/argh/assembling.py b/src/argh/assembling.py index 6ddb150..7baf539 100644 --- a/src/argh/assembling.py +++ b/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__ diff --git a/tests/test_assembling.py b/tests/test_assembling.py index fc5d5bf..c40818b 100644 --- a/tests/test_assembling.py +++ b/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,14 +66,14 @@ 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, @@ -81,7 +81,81 @@ def func(): 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() diff --git a/tests/test_integration.py b/tests/test_integration.py index e7831d9..d403a31 100644 --- a/tests/test_integration.py +++ b/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)