fix: raise exception for non-migrated commands (fixes #206)

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,24 +268,24 @@ 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:
 
 - The undocumented (and untested) argument `dispatch(..., pre_call=x)`
   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`

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"

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,17 +150,49 @@ 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,
             default_value=default_value,
         )
 
         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):
+    ...

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()