From d764de9438c9430dc6a9953ad9fca1b973ea62a8 Mon Sep 17 00:00:00 2001 From: Andy Mikhaylenko Date: Tue, 24 Oct 2023 23:09:32 +0200 Subject: [PATCH 1/5] fix: raise exception for non-migrated commands (fixes #206) --- CHANGES.rst | 92 ++++++++++++++++++++++++---------- pyproject.toml | 2 +- src/argh/assembling.py | 84 +++++++++++++++++++++++-------- tests/test_assembling.py | 8 ++- tests/test_integration.py | 8 ++- tests/test_mapping_policies.py | 45 +++++++++++++---- tests/test_regressions.py | 4 +- 7 files changed, 175 insertions(+), 68 deletions(-) diff --git a/CHANGES.rst b/CHANGES.rst index a6aa698..453bc40 100644 --- a/CHANGES.rst +++ b/CHANGES.rst @@ -2,8 +2,27 @@ Changelog ~~~~~~~~~ -Version 0.30.1 --------------- +Version 0.30.2 (2023-10-24) +--------------------------- + +Bugs fixed: + +- As reported in #204 and #206, the new default name mapping policy in fact + silently changed the CLI API of some scripts: arguments which were previously + translated as CLI options became optional positionals. Although the + instructions were supplied in the release notes, the upgrade may not + necessarily be intentional, so a waste of users' time is quite likely. + + To alleviate this, the default value for `name_mapping_policy` in standard + functions has been changed to `None`; if it's not specified, Argh falls back + to the new default policy, but raises `ArgumentNameMappingError` with + detailed instructions if it sees a non-kwonly argument with a default value. + + Please specify the policy explicitly in order to avoid this error if you need + to infer optional positionals (``nargs="?"``) from function signature. + +Version 0.30.1 (2023-10-23) +--------------------------- Bugs fixed: @@ -21,8 +40,8 @@ Other changes: - Added `py.typed` marker file for :pep:`561`. -Version 0.30.0 --------------- +Version 0.30.0 (2023-10-21) +--------------------------- Backwards incompatible changes: @@ -106,8 +125,8 @@ Enhancements: Please check API docs on :class:`argh.assembling.NameMappingPolicy` for details. -Version 0.29.4 --------------- +Version 0.29.4 (2023-09-23) +--------------------------- Bugs fixed: @@ -118,8 +137,8 @@ Versions 0.29.1 through 0.29.3 Technical releases for packaging purposes. No changes in functionality. -Version 0.29.0 --------------- +Version 0.29.0 (2023-09-03) +--------------------------- Backwards incompatible changes: @@ -151,13 +170,13 @@ Other changes: - Avoid depending on iocapture by using pytest's built-in feature (#177) -Version 0.28.1 --------------- +Version 0.28.1 (2023-02-16) +--------------------------- - Fixed bugs in tests (#171, #172) -Version 0.28.0 --------------- +Version 0.28.0 (2023-02-15) +--------------------------- A major cleanup. @@ -202,23 +221,23 @@ Deprecated features, to be removed in v.0.30: - Added deprecation warnings for some arguments deprecated back in v.0.26. -Version 0.27.2 --------------- +Version 0.27.2 (2023-02-09) +--------------------------- Minor packaging fix: * chore: include file required by tox.ini in the sdist (#155) -Version 0.27.1 --------------- +Version 0.27.1 (2023-02-09) +--------------------------- Minor building and packaging fixes: * docs: add Read the Docs config (#160) * chore: include tox.ini in the sdist (#155) -Version 0.27.0 --------------- +Version 0.27.0 (2023-02-09) +--------------------------- This is the last version to support Python 2.7. @@ -249,15 +268,15 @@ Other changes: - Fixed typos and links in documentation (PR #110, #116, #156). - Switched CI to Github Actions (PR #153). -Version 0.26.2 --------------- +Version 0.26.2 (2016-05-11) +--------------------------- - Removed official support for Python 3.4, added for 3.5. - Various tox-related improvements for development. - Improved documentation. -Version 0.26.1 --------------- +Version 0.26.1 (2014-10-30) +--------------------------- Fixed bugs: @@ -265,8 +284,8 @@ Fixed bugs: was broken; fixing because at least one important app depends on it (issue #63). -Version 0.26 ------------- +Version 0.26 (2014-10-27) +------------------------- This release is intended to be the last one before 1.0. Therefore a major cleanup was done. This **breaks backward compatibility**. If your code is @@ -320,8 +339,8 @@ Fixed bugs: - Help formatter was broken for arguments with empty strings as default values (issue #76). -Version 0.25 ------------- +Version 0.25 (2014-07-05) +------------------------- - Added EntryPoint class as another way to assemble functions (issue #59). @@ -336,3 +355,24 @@ Version 0.25 - Function docstrings are now displayed verbatim in the help (issue #64). - Argh's dispatching now should work properly in Cython. + +Versions 0.2 through 0.24 +------------------------- + +A few years of development without a changelog 🫠 + +Fortunately, a curious reader can always refer to commit messages and +changesets. + +Version 0.1 (2010-11-12) +------------------------ + +The first version! A single file with 182 lines of code including +documentation :) It featured subparsers and had the `@arg` decorator which was +basically a deferred `ArgumentParser.add_argument()` call. + +Functions and classes: + +* class `ArghParser` +* functions `add_commands()` and `dispatch()` +* decorators `@arg` and `@plain_signature` diff --git a/pyproject.toml b/pyproject.toml index 04f6c2f..92a3906 100644 --- a/pyproject.toml +++ b/pyproject.toml @@ -4,7 +4,7 @@ build-backend = "flit_core.buildapi" [project] name = "argh" -version = "0.30.1" +version = "0.30.2" description = "An unobtrusive argparse wrapper with natural syntax" readme = "README.rst" requires-python = ">=3.8" diff --git a/src/argh/assembling.py b/src/argh/assembling.py index 93c7f19..381fd61 100644 --- a/src/argh/assembling.py +++ b/src/argh/assembling.py @@ -14,6 +14,7 @@ Functions and classes to properly assemble your commands in a parser. """ import inspect +import textwrap from argparse import OPTIONAL, ZERO_OR_MORE, ArgumentParser from collections import OrderedDict from enum import Enum @@ -105,14 +106,12 @@ def func(alpha, beta=1, *, gamma, delta=2): ... def infer_argspecs_from_function( function: Callable, - name_mapping_policy: Optional[ - NameMappingPolicy - ] = NameMappingPolicy.BY_NAME_IF_KWONLY, + name_mapping_policy: Optional[NameMappingPolicy] = None, ) -> Iterator[ParserAddArgumentSpec]: if getattr(function, ATTR_EXPECTS_NAMESPACE_OBJECT, False): return - if name_mapping_policy not in NameMappingPolicy: + if name_mapping_policy and name_mapping_policy not in NameMappingPolicy: raise NotImplementedError(f"Unknown name mapping policy {name_mapping_policy}") func_spec = get_arg_spec(function) @@ -151,6 +150,38 @@ def _make_cli_arg_names_options(arg_name) -> Tuple[List[str], List[str]]: ) default_value = default_by_arg_name.get(arg_name, NotDefined) + if default_value != NotDefined and not name_mapping_policy: + # TODO: remove this after something like v.0.31+2 + raise ArgumentNameMappingError( + textwrap.dedent( + f""" + Argument "{arg_name}" in function "{function.__name__}" + is not keyword-only but has a default value. + + Please note that since Argh v.0.30 the default name mapping + policy has changed. + + More information: + https://argh.readthedocs.io/en/latest/changes.html#version-0-30-0-2023-10-21 + + You need to upgrade your functions so that the arguments + that have default values become keyword-only: + + f(x=1) -> f(*, x=1) + + If you actually want an optional positional argument, + please set the name mapping policy explicitly to `BY_NAME_IF_KWONLY`. + + If you choose to postpone the migration, you have two options: + + a) set the policy explicitly to `BY_NAME_IF_HAS_DEFAULT`; + b) pin Argh version to 0.29 until you are ready to migrate. + + Thank you for understanding! + """ + ).strip() + ) + arg_spec = ParserAddArgumentSpec( func_arg_name=arg_name, cli_arg_names=cli_arg_names_positional, @@ -158,10 +189,10 @@ def _make_cli_arg_names_options(arg_name) -> Tuple[List[str], List[str]]: ) if default_value != NotDefined: - if name_mapping_policy == NameMappingPolicy.BY_NAME_IF_KWONLY: - arg_spec.nargs = OPTIONAL - else: + if name_mapping_policy == NameMappingPolicy.BY_NAME_IF_HAS_DEFAULT: arg_spec.cli_arg_names = cli_arg_names_options + else: + arg_spec.nargs = OPTIONAL yield arg_spec @@ -181,13 +212,13 @@ def _make_cli_arg_names_options(arg_name) -> Tuple[List[str], List[str]]: default_value=default_value, ) - if name_mapping_policy == NameMappingPolicy.BY_NAME_IF_KWONLY: + if name_mapping_policy == NameMappingPolicy.BY_NAME_IF_HAS_DEFAULT: + if default_value != NotDefined: + arg_spec.cli_arg_names = cli_arg_names_options + else: arg_spec.cli_arg_names = cli_arg_names_options if default_value == NotDefined: arg_spec.is_required = True - else: - if default_value != NotDefined: - arg_spec.cli_arg_names = cli_arg_names_options yield arg_spec @@ -252,7 +283,7 @@ def guess_extra_parser_add_argument_spec_kwargs( def set_default_command( parser, function: Callable, - name_mapping_policy: NameMappingPolicy = NameMappingPolicy.BY_NAME_IF_KWONLY, + name_mapping_policy: Optional[NameMappingPolicy] = None, ) -> None: """ Sets default command (i.e. a function) for given parser. @@ -267,17 +298,25 @@ def set_default_command( :name_mapping_policy: The policy to use when mapping function arguments onto CLI arguments. - See :class:`.NameMappingPolicy`. + See :class:`.NameMappingPolicy`. If not defined explicitly, + :meth:`.NameMappingPolicy.BY_NAME_IF_KWONLY` is used. .. versionadded:: 0.30 + .. versionchanged:: 0.30.2 + Raises `ArgumentNameMappingError` if the policy was not explicitly + defined and a non-kwonly argument has a default value. The reason + is that it's very likely to be a case of non-migrated code where + the argument was intended to be mapped onto a CLI option. It's + better to fail explicitly than to silently change the CLI API. + .. note:: If there are both explicitly declared arguments (e.g. via :func:`~argh.decorators.arg`) and ones inferred from the function signature, declared ones will be merged into inferred ones. If an argument does not conform to the function signature, - `AssemblingError` is raised. + `ArgumentNameMappingError` is raised. .. note:: @@ -294,7 +333,7 @@ def set_default_command( ) if declared_args and not inferred_args and not has_varkw: - raise AssemblingError( + raise ArgumentNameMappingError( f"{function.__name__}: cannot extend argument declarations " "for an endpoint function that takes no arguments." ) @@ -309,9 +348,8 @@ def set_default_command( declared_args=declared_args, has_varkw=has_varkw, ) - except AssemblingError as exc: - print(exc) - raise AssemblingError(f"{function.__name__}: {exc}") from exc + except ArgumentNameMappingError as exc: + raise ArgumentNameMappingError(f"{function.__name__}: {exc}") from exc # add the fully formed argument specs to the parser for spec in parser_add_argument_specs: @@ -413,7 +451,7 @@ def _merge_inferred_and_declared_args( kinds = {True: "positional", False: "optional"} kind_inferred = kinds[infr_positional] kind_declared = kinds[decl_positional] - raise AssemblingError( + raise ArgumentNameMappingError( f'argument "{func_arg_name}" declared as {kind_inferred} ' f"(in function signature) and {kind_declared} (via decorator). " "If you've just migrated from Argh v.0.29, please check " @@ -438,7 +476,7 @@ def _merge_inferred_and_declared_args( ) msg_flags = ", ".join(declared_spec.cli_arg_names) msg_signature = ", ".join("/".join(x) for x in dest_option_strings) - raise AssemblingError( + raise ArgumentNameMappingError( f"argument {msg_flags} does not fit " f"function signature: {msg_signature}" ) @@ -458,7 +496,7 @@ def _is_positional(args: List[str], prefix_chars: str = "-") -> bool: def add_commands( parser: ArgumentParser, functions: List[Callable], - name_mapping_policy: NameMappingPolicy = NameMappingPolicy.BY_NAME_IF_KWONLY, + name_mapping_policy: Optional[NameMappingPolicy] = None, group_name: Optional[str] = None, group_kwargs: Optional[Dict[str, Any]] = None, func_kwargs: Optional[Dict[str, Any]] = None, @@ -601,3 +639,7 @@ def add_subcommands( """ add_commands(parser, functions, group_name=group_name, group_kwargs=group_kwargs) + + +class ArgumentNameMappingError(AssemblingError): + ... diff --git a/tests/test_assembling.py b/tests/test_assembling.py index ed9d47a..dd38586 100644 --- a/tests/test_assembling.py +++ b/tests/test_assembling.py @@ -88,7 +88,9 @@ def func(pos_int_default=123): ... parser = argh.ArghParser(prog="test") - parser.set_default_command(func) + parser.set_default_command( + func, name_mapping_policy=NameMappingPolicy.BY_NAME_IF_KWONLY + ) assert parser.format_usage() == "usage: test [-h] [pos-int-default]\n" assert "pos-int-default 123" in parser.format_help() @@ -98,7 +100,9 @@ def func(pos_bool_default=False): ... parser = argh.ArghParser(prog="test") - parser.set_default_command(func) + parser.set_default_command( + func, name_mapping_policy=NameMappingPolicy.BY_NAME_IF_KWONLY + ) assert parser.format_usage() == "usage: test [-h] [pos-bool-default]\n" assert "pos-bool-default False" in parser.format_help() diff --git a/tests/test_integration.py b/tests/test_integration.py index b5206e7..b2f392d 100644 --- a/tests/test_integration.py +++ b/tests/test_integration.py @@ -786,7 +786,7 @@ def func(): def test_prog(capsys: pytest.CaptureFixture[str]): "Program name propagates from sys.argv[0]" - def cmd(foo=1): + def cmd(*, foo=1): return foo parser = DebugArghParser() @@ -832,9 +832,8 @@ class UnsuitablePolicyContainer(Enum): def test_add_commands_no_overrides1(capsys: pytest.CaptureFixture[str]): - def first_func(foo=123): + def first_func(*, foo=123): """Owl stretching time""" - pass def second_func(): pass @@ -867,7 +866,6 @@ def second_func(): def test_add_commands_no_overrides2(capsys: pytest.CaptureFixture[str]): def first_func(*, foo=123): """Owl stretching time""" - pass def second_func(): pass @@ -899,7 +897,7 @@ def test_add_commands_group_overrides1(capsys: pytest.CaptureFixture[str]): whatever was specified on function level. """ - def first_func(foo=123): + def first_func(*, foo=123): """Owl stretching time""" return foo diff --git a/tests/test_mapping_policies.py b/tests/test_mapping_policies.py index 7635e14..a5e5204 100644 --- a/tests/test_mapping_policies.py +++ b/tests/test_mapping_policies.py @@ -1,13 +1,19 @@ import sys from argparse import ArgumentParser, Namespace -from typing import Callable, List +from typing import Callable, List, Optional import pytest -from argh.assembling import NameMappingPolicy, infer_argspecs_from_function +from argh.assembling import ( + ArgumentNameMappingError, + NameMappingPolicy, + infer_argspecs_from_function, +) + +POLICIES = list(NameMappingPolicy) + [None] -@pytest.mark.parametrize("name_mapping_policy", list(NameMappingPolicy)) +@pytest.mark.parametrize("name_mapping_policy", POLICIES) def test_no_args(name_mapping_policy) -> None: def func() -> None: ... @@ -16,7 +22,7 @@ def func() -> None: assert_usage(parser, "usage: test [-h]\n") -@pytest.mark.parametrize("name_mapping_policy", list(NameMappingPolicy)) +@pytest.mark.parametrize("name_mapping_policy", POLICIES) def test_one_positional(name_mapping_policy) -> None: def func(alpha: str) -> str: return f"{alpha}" @@ -26,7 +32,7 @@ def func(alpha: str) -> str: assert_parsed(parser, ["hello"], Namespace(alpha="hello")) -@pytest.mark.parametrize("name_mapping_policy", list(NameMappingPolicy)) +@pytest.mark.parametrize("name_mapping_policy", POLICIES) def test_two_positionals(name_mapping_policy) -> None: def func(alpha: str, beta: str) -> str: return f"{alpha}, {beta}" @@ -62,16 +68,19 @@ def func(alpha: str, beta: int = 123) -> str: assert_parsed(parser, ["one", "two"], Namespace(alpha="one", beta="two")) -@pytest.mark.parametrize("name_mapping_policy", list(NameMappingPolicy)) +@pytest.mark.parametrize("name_mapping_policy", POLICIES) def test_varargs(name_mapping_policy) -> None: def func(*file_paths) -> str: return f"{file_paths}" parser = _make_parser_for_function(func, name_mapping_policy=name_mapping_policy) expected_usage = "usage: test [-h] [file-paths ...]\n" + + # TODO: remove once we drop support for Python 3.8 if sys.version_info < (3, 9): # https://github.com/python/cpython/issues/82619 expected_usage = "usage: test [-h] [file-paths [file-paths ...]]\n" + assert_usage(parser, expected_usage) @@ -80,6 +89,7 @@ def func(*file_paths) -> str: [ (NameMappingPolicy.BY_NAME_IF_HAS_DEFAULT, "usage: test [-h] alpha beta\n"), (NameMappingPolicy.BY_NAME_IF_KWONLY, "usage: test [-h] -b BETA alpha\n"), + (None, "usage: test [-h] -b BETA alpha\n"), ], ) def test_varargs_between_positional_and_kwonly__no_defaults( @@ -112,13 +122,26 @@ def func(alpha: int = 1, *, beta: int = 2) -> str: assert_usage(parser, expected_usage) -def test_kwargs() -> None: +def test_varargs_between_positional_and_kwonly__with_defaults__no_explicit_policy() -> ( + None +): + def func(alpha: int = 1, *, beta: int = 2) -> str: + return f"{alpha}, {beta}" + + with pytest.raises(ArgumentNameMappingError) as exc: + _make_parser_for_function(func, name_mapping_policy=None) + assert ( + 'Argument "alpha" in function "func"\n' + "is not keyword-only but has a default value." + ) in str(exc.value) + + +@pytest.mark.parametrize("name_mapping_policy", POLICIES) +def test_kwargs(name_mapping_policy) -> None: def func(**kwargs) -> str: return f"{kwargs}" - parser = _make_parser_for_function( - func, name_mapping_policy=NameMappingPolicy.BY_NAME_IF_KWONLY - ) + parser = _make_parser_for_function(func, name_mapping_policy=name_mapping_policy) assert_usage(parser, "usage: test [-h]\n") @@ -145,7 +168,7 @@ def func(alpha: str, beta: int = 1, *, gamma: str, delta: int = 2) -> str: def _make_parser_for_function( func: Callable, - name_mapping_policy: NameMappingPolicy = NameMappingPolicy.BY_NAME_IF_HAS_DEFAULT, + name_mapping_policy: Optional[NameMappingPolicy] = None, ) -> ArgumentParser: parser = ArgumentParser(prog="test") parser_add_argument_specs = infer_argspecs_from_function( diff --git a/tests/test_regressions.py b/tests/test_regressions.py index 6e4314b..bb49e70 100644 --- a/tests/test_regressions.py +++ b/tests/test_regressions.py @@ -130,7 +130,7 @@ def test_regression_issue76(): This is also tested in integration tests but in a different way. """ - def cmd(foo=""): + def cmd(*, foo=""): pass parser = DebugArghParser() @@ -166,7 +166,7 @@ def test_regression_issue204(): We should avoid `deepcopy()` in standard operations. """ - def func(x: TextIO = sys.stdout) -> None: + def func(*, x: TextIO = sys.stdout) -> None: ... parser = DebugArghParser() From 6992b31192fa69af6ba72bdbfea4c7f01b209a0e Mon Sep 17 00:00:00 2001 From: Andy Mikhaylenko Date: Mon, 30 Oct 2023 22:23:42 +0100 Subject: [PATCH 2/5] =?UTF-8?q?fix:=20regression=20=E2=80=94=20`@arg`=20de?= =?UTF-8?q?co=20failing=20with=20underscore=20in=20positional=20arg=20name?= =?UTF-8?q?=20(fixes=20#208)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit --- CHANGES.rst | 8 ++++++++ pyproject.toml | 2 +- src/argh/decorators.py | 3 ++- tests/test_decorators.py | 4 ++-- tests/test_integration.py | 23 +++++++++++++++-------- tests/test_regressions.py | 9 +++++++++ 6 files changed, 37 insertions(+), 12 deletions(-) diff --git a/CHANGES.rst b/CHANGES.rst index 453bc40..426a56d 100644 --- a/CHANGES.rst +++ b/CHANGES.rst @@ -2,6 +2,14 @@ Changelog ~~~~~~~~~ +Version 0.30.3 (2023-10-30) +--------------------------- + +Bugs fixed: + +- Regression: a positional argument with an underscore used in `@arg` decorator + would cause Argh fail on the assembling stage. (#208) + Version 0.30.2 (2023-10-24) --------------------------- diff --git a/pyproject.toml b/pyproject.toml index 92a3906..26c7cce 100644 --- a/pyproject.toml +++ b/pyproject.toml @@ -4,7 +4,7 @@ build-backend = "flit_core.buildapi" [project] name = "argh" -version = "0.30.2" +version = "0.30.3" description = "An unobtrusive argparse wrapper with natural syntax" readme = "README.rst" requires-python = ">=3.8" diff --git a/src/argh/decorators.py b/src/argh/decorators.py index aad081b..10ede10 100644 --- a/src/argh/decorators.py +++ b/src/argh/decorators.py @@ -141,10 +141,11 @@ def wrapper(func: Callable) -> Callable: raise CliArgToFuncArgGuessingError("at least one CLI arg must be defined") func_arg_name = naive_guess_func_arg_name(args) + cli_arg_names = [name.replace("_", "-") for name in args] completer = kwargs.pop("completer", None) spec = ParserAddArgumentSpec.make_from_kwargs( func_arg_name=func_arg_name, - cli_arg_names=args, + cli_arg_names=cli_arg_names, parser_add_argument_kwargs=kwargs, ) if completer: diff --git a/tests/test_decorators.py b/tests/test_decorators.py index 7ac1d84..b813069 100644 --- a/tests/test_decorators.py +++ b/tests/test_decorators.py @@ -33,7 +33,7 @@ def func(): assert attrs == [ ParserAddArgumentSpec( func_arg_name="foo", - cli_arg_names=("foo",), + cli_arg_names=["foo"], nargs="+", other_add_parser_kwargs={ "help": "my help", @@ -41,7 +41,7 @@ def func(): ), ParserAddArgumentSpec( func_arg_name="bar", - cli_arg_names=("--bar",), + cli_arg_names=["--bar"], default_value=1, ), ] diff --git a/tests/test_integration.py b/tests/test_integration.py index b2f392d..e70db4f 100644 --- a/tests/test_integration.py +++ b/tests/test_integration.py @@ -735,14 +735,21 @@ def remind( parser = DebugArghParser() parser.set_default_command(remind) - assert "Basil" in parser.format_help() - assert "Moose" in parser.format_help() - assert "creatures" in parser.format_help() - - # explicit help message is not obscured by the implicit one... - assert "remarkable animal" in parser.format_help() - # ...but is still present - assert "it can speak" in parser.format_help() + help_normalised = re.sub(r"\s+", " ", parser.format_help()) + + assert "name 'Basil'" in help_normalised + assert "-t TASK, --task TASK 'hang the Moose'" in help_normalised + assert ( + "-r REASON, --reason REASON 'there are creatures living in it'" + in help_normalised + ) + + # explicit help message is not obscured by the implicit one + # but is still present + assert ( + "-n NOTE, --note NOTE why is it a remarkable animal? " + "(default: 'it can speak English')" + ) in help_normalised def test_default_arg_values_in_help__regression(): diff --git a/tests/test_regressions.py b/tests/test_regressions.py index bb49e70..c19115f 100644 --- a/tests/test_regressions.py +++ b/tests/test_regressions.py @@ -171,3 +171,12 @@ def func(*, x: TextIO = sys.stdout) -> None: parser = DebugArghParser() parser.set_default_command(func) + + +def test_regression_issue208(): + @argh.arg("foo_bar", help="fooooo") + def func(foo_bar): + return foo_bar + + parser = DebugArghParser() + parser.set_default_command(func) From 1f98369c8ce744d02e9821f7926ca3fb748cfa3f Mon Sep 17 00:00:00 2001 From: Andy Mikhaylenko Date: Sat, 4 Nov 2023 17:54:34 +0100 Subject: [PATCH 3/5] chore: convert arg name error to deprecation warning --- CHANGES.rst | 27 +++++++++++++++++ src/argh/assembling.py | 55 ++++++++++++++++++++-------------- tests/test_mapping_policies.py | 45 ++++++++++++++++++---------- 3 files changed, 90 insertions(+), 37 deletions(-) diff --git a/CHANGES.rst b/CHANGES.rst index 426a56d..79cc368 100644 --- a/CHANGES.rst +++ b/CHANGES.rst @@ -2,6 +2,33 @@ Changelog ~~~~~~~~~ +Version 0.30.4 (2023-11-04) +--------------------------- + +There were complaints about the lack of a deprecation cycle for the legacy name +mapping policy. This version addresses the issue: + +- The handling introduced in v.0.30.2 (raising an exception for clarity) + is retained for cases when no name mapping policy is specified but function + signature contains defaults in non-kwonly args **and kwonly args are also + defined**:: + + def main(alpha, beta=1, *, gamma=2): # error — explicit policy required + + In a similar case but when **kwonly args are not defined** Argh now assumes + the legacy name mapping policy (`BY_NAME_IF_HAS_DEFAULT`) and merely issues + a deprecation warning with the same message as the exception mentioned above:: + + def main(alpha, beta=2): # `[-b BETA] alpha` + DeprecationWarning + + This ensures that most of the old scripts still work the same way despite the + new policy being used by default and enforced in cases when it's impossible + to resolve the mapping conflict. + + Please note that this "soft" handling is to be removed in version v0.33 + (or v1.0 if the former is not deemed necessary). The new name mapping policy + will be used by default without warnings, like in v0.30. + Version 0.30.3 (2023-10-30) --------------------------- diff --git a/src/argh/assembling.py b/src/argh/assembling.py index 381fd61..0d2786b 100644 --- a/src/argh/assembling.py +++ b/src/argh/assembling.py @@ -15,6 +15,7 @@ """ import inspect import textwrap +import warnings from argparse import OPTIONAL, ZERO_OR_MORE, ArgumentParser from collections import OrderedDict from enum import Enum @@ -115,6 +116,7 @@ def infer_argspecs_from_function( raise NotImplementedError(f"Unknown name mapping policy {name_mapping_policy}") func_spec = get_arg_spec(function) + has_kwonly = bool(func_spec.kwonlyargs) default_by_arg_name: Dict[str, Any] = dict( zip(reversed(func_spec.args), reversed(func_spec.defaults or tuple())) @@ -151,36 +153,45 @@ def _make_cli_arg_names_options(arg_name) -> Tuple[List[str], List[str]]: default_value = default_by_arg_name.get(arg_name, NotDefined) if default_value != NotDefined and not name_mapping_policy: - # TODO: remove this after something like v.0.31+2 - raise ArgumentNameMappingError( - textwrap.dedent( - f""" - Argument "{arg_name}" in function "{function.__name__}" - is not keyword-only but has a default value. + message = textwrap.dedent( + f""" + Argument "{arg_name}" in function "{function.__name__}" + is not keyword-only but has a default value. - Please note that since Argh v.0.30 the default name mapping - policy has changed. + Please note that since Argh v.0.30 the default name mapping + policy has changed. - More information: - https://argh.readthedocs.io/en/latest/changes.html#version-0-30-0-2023-10-21 + More information: + https://argh.readthedocs.io/en/latest/changes.html#version-0-30-0-2023-10-21 - You need to upgrade your functions so that the arguments - that have default values become keyword-only: + You need to upgrade your functions so that the arguments + that have default values become keyword-only: - f(x=1) -> f(*, x=1) + f(x=1) -> f(*, x=1) - If you actually want an optional positional argument, - please set the name mapping policy explicitly to `BY_NAME_IF_KWONLY`. + If you actually want an optional positional argument, + please set the name mapping policy explicitly to `BY_NAME_IF_KWONLY`. - If you choose to postpone the migration, you have two options: + If you choose to postpone the migration, you have two options: - a) set the policy explicitly to `BY_NAME_IF_HAS_DEFAULT`; - b) pin Argh version to 0.29 until you are ready to migrate. + a) set the policy explicitly to `BY_NAME_IF_HAS_DEFAULT`; + b) pin Argh version to 0.29 until you are ready to migrate. - Thank you for understanding! - """ - ).strip() - ) + Thank you for understanding! + """ + ).strip() + + # Assume legacy policy and show a warning if the signature is + # simple (without kwonly args) so that the script continues working + # but the author is urged to upgrade it. + # When it cannot be auto-resolved (kwonly args mixed with old-style + # ones but no policy specified), throw an error. + # + # TODO: remove in v.0.33 if it happens, otherwise in v1.0. + if has_kwonly: + raise ArgumentNameMappingError(message) + warnings.warn(DeprecationWarning(message)) + name_mapping_policy = NameMappingPolicy.BY_NAME_IF_HAS_DEFAULT arg_spec = ParserAddArgumentSpec( func_arg_name=arg_name, diff --git a/tests/test_mapping_policies.py b/tests/test_mapping_policies.py index a5e5204..6b6ae8e 100644 --- a/tests/test_mapping_policies.py +++ b/tests/test_mapping_policies.py @@ -19,7 +19,7 @@ def func() -> None: ... parser = _make_parser_for_function(func, name_mapping_policy=name_mapping_policy) - assert_usage(parser, "usage: test [-h]\n") + assert_usage(parser, "usage: test [-h]") @pytest.mark.parametrize("name_mapping_policy", POLICIES) @@ -28,7 +28,7 @@ def func(alpha: str) -> str: return f"{alpha}" parser = _make_parser_for_function(func, name_mapping_policy=name_mapping_policy) - assert_usage(parser, "usage: test [-h] alpha\n") + assert_usage(parser, "usage: test [-h] alpha") assert_parsed(parser, ["hello"], Namespace(alpha="hello")) @@ -38,7 +38,7 @@ def func(alpha: str, beta: str) -> str: return f"{alpha}, {beta}" parser = _make_parser_for_function(func, name_mapping_policy=name_mapping_policy) - assert_usage(parser, "usage: test [-h] alpha beta\n") + assert_usage(parser, "usage: test [-h] alpha beta") assert_parsed(parser, ["one", "two"], Namespace(alpha="one", beta="two")) @@ -47,9 +47,9 @@ def func(alpha: str, beta: str) -> str: [ ( NameMappingPolicy.BY_NAME_IF_HAS_DEFAULT, - "usage: test [-h] [-b BETA] alpha\n", + "usage: test [-h] [-b BETA] alpha", ), - (NameMappingPolicy.BY_NAME_IF_KWONLY, "usage: test [-h] alpha [beta]\n"), + (NameMappingPolicy.BY_NAME_IF_KWONLY, "usage: test [-h] alpha [beta]"), ], ) def test_two_positionals_one_with_default(name_mapping_policy, expected_usage) -> None: @@ -74,12 +74,12 @@ def func(*file_paths) -> str: return f"{file_paths}" parser = _make_parser_for_function(func, name_mapping_policy=name_mapping_policy) - expected_usage = "usage: test [-h] [file-paths ...]\n" + expected_usage = "usage: test [-h] [file-paths ...]" # TODO: remove once we drop support for Python 3.8 if sys.version_info < (3, 9): # https://github.com/python/cpython/issues/82619 - expected_usage = "usage: test [-h] [file-paths [file-paths ...]]\n" + expected_usage = "usage: test [-h] [file-paths [file-paths ...]]" assert_usage(parser, expected_usage) @@ -87,9 +87,9 @@ def func(*file_paths) -> str: @pytest.mark.parametrize( "name_mapping_policy,expected_usage", [ - (NameMappingPolicy.BY_NAME_IF_HAS_DEFAULT, "usage: test [-h] alpha beta\n"), - (NameMappingPolicy.BY_NAME_IF_KWONLY, "usage: test [-h] -b BETA alpha\n"), - (None, "usage: test [-h] -b BETA alpha\n"), + (NameMappingPolicy.BY_NAME_IF_HAS_DEFAULT, "usage: test [-h] alpha beta"), + (NameMappingPolicy.BY_NAME_IF_KWONLY, "usage: test [-h] -b BETA alpha"), + (None, "usage: test [-h] -b BETA alpha"), ], ) def test_varargs_between_positional_and_kwonly__no_defaults( @@ -107,9 +107,9 @@ def func(alpha, *, beta) -> str: [ ( NameMappingPolicy.BY_NAME_IF_HAS_DEFAULT, - "usage: test [-h] [-a ALPHA] [-b BETA]\n", + "usage: test [-h] [-a ALPHA] [-b BETA]", ), - (NameMappingPolicy.BY_NAME_IF_KWONLY, "usage: test [-h] [-b BETA] [alpha]\n"), + (NameMappingPolicy.BY_NAME_IF_KWONLY, "usage: test [-h] [-b BETA] [alpha]"), ], ) def test_varargs_between_positional_and_kwonly__with_defaults( @@ -136,13 +136,26 @@ def func(alpha: int = 1, *, beta: int = 2) -> str: ) in str(exc.value) +# TODO: remove in v.0.33 if it happens, otherwise in v1.0. +def test_positional_with_defaults_without_kwonly__no_explicit_policy() -> ( + None +): + def func(alpha: str, beta: int = 1) -> str: + return f"{alpha} {beta}" + + message_pattern = 'Argument "beta" in function "func"\nis not keyword-only but has a default value.' + with pytest.warns(DeprecationWarning, match=message_pattern): + parser = _make_parser_for_function(func, name_mapping_policy=None) + assert_usage(parser, "usage: test [-h] [-b BETA] alpha") + + @pytest.mark.parametrize("name_mapping_policy", POLICIES) def test_kwargs(name_mapping_policy) -> None: def func(**kwargs) -> str: return f"{kwargs}" parser = _make_parser_for_function(func, name_mapping_policy=name_mapping_policy) - assert_usage(parser, "usage: test [-h]\n") + assert_usage(parser, "usage: test [-h]") @pytest.mark.parametrize( @@ -150,11 +163,11 @@ def func(**kwargs) -> str: [ ( NameMappingPolicy.BY_NAME_IF_HAS_DEFAULT, - "usage: test [-h] [-b BETA] [-d DELTA] alpha gamma\n", + "usage: test [-h] [-b BETA] [-d DELTA] alpha gamma", ), ( NameMappingPolicy.BY_NAME_IF_KWONLY, - "usage: test [-h] -g GAMMA [-d DELTA] alpha [beta]\n", + "usage: test [-h] -g GAMMA [-d DELTA] alpha [beta]", ), ], ) @@ -183,6 +196,8 @@ def _make_parser_for_function( def assert_usage(parser: ArgumentParser, expected_usage: str) -> None: + if not expected_usage.endswith("\n"): + expected_usage += "\n" assert expected_usage == parser.format_usage() From 4ab2a0b2f8980011fe1fe137be04a27dc1b59d23 Mon Sep 17 00:00:00 2001 From: Andy Mikhaylenko Date: Sat, 4 Nov 2023 17:54:59 +0100 Subject: [PATCH 4/5] chore: bump version --- pyproject.toml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/pyproject.toml b/pyproject.toml index 26c7cce..499cb04 100644 --- a/pyproject.toml +++ b/pyproject.toml @@ -4,7 +4,7 @@ build-backend = "flit_core.buildapi" [project] name = "argh" -version = "0.30.3" +version = "0.30.4" description = "An unobtrusive argparse wrapper with natural syntax" readme = "README.rst" requires-python = ">=3.8" From 7a9592fa362a65ab64f888ea46a362c44d98418f Mon Sep 17 00:00:00 2001 From: Andy Mikhaylenko Date: Sat, 4 Nov 2023 17:56:29 +0100 Subject: [PATCH 5/5] style: formatting --- tests/test_mapping_policies.py | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/tests/test_mapping_policies.py b/tests/test_mapping_policies.py index 6b6ae8e..db67e71 100644 --- a/tests/test_mapping_policies.py +++ b/tests/test_mapping_policies.py @@ -137,9 +137,7 @@ def func(alpha: int = 1, *, beta: int = 2) -> str: # TODO: remove in v.0.33 if it happens, otherwise in v1.0. -def test_positional_with_defaults_without_kwonly__no_explicit_policy() -> ( - None -): +def test_positional_with_defaults_without_kwonly__no_explicit_policy() -> None: def func(alpha: str, beta: int = 1) -> str: return f"{alpha} {beta}"