Merge pull request #210 from neithere/v0.30.4

Release v0.30.4

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

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"

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,

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,22 +74,22 @@ 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)
 
 
 @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,25 +136,36 @@ 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(
     "name_mapping_policy,expected_usage",
     [
         (
             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 +194,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()