From 1f5d7154adf7f3d16c70301511ba2f610bcb562a Mon Sep 17 00:00:00 2001 From: Andy Mikhaylenko Date: Tue, 24 Oct 2023 23:09:32 +0200 Subject: [PATCH] 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()