Deprecate parameter names that are: not a Python identifier, or a Python Keyword, or not lower-cased

CHANGES.md

@@ -1,3 +1,12 @@
+## Version 8.6.0
+
+Unreleased
+
+- A parameter name that is not a valid Python identifier, or that is a Python
+  keyword, is deprecated and raises `TypeError` in Click 9.0. {pr}`3866`
+- An {class}`Option` name written as a Python identifier is deprecated when it
+  is not already lower-cased. {pr}`3866`
+
 ## Version 8.5.1
 
 Unreleased

docs/upgrade-guides.md

@@ -13,6 +13,8 @@ This guide assumes the user is on version 8.3.X.
 ### Deprecations
 
 For each deprecation, provide a brief explanation, and direct users to new function / class if available.
+- A parameter name that is not a valid Python identifier, or that is a [Python keyword](https://docs.python.org/3/reference/lexical_analysis.html#keywords), is deprecated and raises `TypeError` in 9.0. [`str.isidentifier()`](https://docs.python.org/3/library/stdtypes.html#str.isidentifier) decides the first case, so `click.argument("0foo")` fails it. It accepts a keyword, so `click.option("--from")` names a parameter `from`, which no callback can declare. [Soft keywords](https://docs.python.org/3/reference/lexical_analysis.html#soft-keywords) such as `match` and `type` are contextual and unaffected, and `--True` and `--None` lower case out of the keyword set. To migrate, pass an explicit name (`click.option("--from", "source")`, `click.option("--0-file", "zero_file")`). An argument takes one declaration and has no explicit-name channel, so rename it and pass `metavar` to keep its old display (`click.argument("zero_file", metavar="0-FILE")`). See [#3827](https://github.com/pallets/click/pull/3827) and [#3866](https://github.com/pallets/click/pull/3866).
+- An option name written as a Python identifier is deprecated when it is not already lower cased: `click.option("--x", "Foo_Bar")` names `foo_bar` rather than `Foo_Bar`. To migrate, spell the name the way the callback declares it. See [#3827](https://github.com/pallets/click/pull/3827).
 - `CliRunner.isolated_filesystem()` is deprecated and will be removed in Click 9.0. The helper predates Python 3 and modern pytest, and it relies on `os.chdir`, which mutates process-global state and is therefore not thread-safe. Replace it with a temporary directory (`tempfile.TemporaryDirectory`, or pytest's `tmp_path` fixture) and pass absolute paths to the command instead of relying on the current working directory. To run commands in parallel, use process-based isolation (such as `pytest-xdist`) rather than threads, since `CliRunner.invoke()` also redirects the process-global standard streams and other interpreter-wide state. See [#3700](https://github.com/pallets/click/issues/3700), [#3501](https://github.com/pallets/click/issues/3501) and the [testing guide](testing.md#running-tests-in-parallel).
 
 ### Removals with prior deprecation

pyproject.toml

@@ -1,6 +1,6 @@
 [project]
 name = "click"
-version = "8.5.1.dev"
+version = "8.6.0.dev"
 description = "Composable command line interface toolkit"
 readme = "README.md"
 license = "BSD-3-Clause"

src/click/core.py

@@ -18,6 +18,7 @@
 from gettext import gettext as _
 from gettext import ngettext
 from itertools import repeat
+from types import FrameType
 from types import TracebackType
 
 from . import types
@@ -104,6 +105,26 @@ def _echo_aborted() -> None:
     echo(_("Aborted!"), file=sys.stderr)
 
 
+def _outside_click_stacklevel() -> int:
+    """Depth of the first stack frame outside Click.
+
+    .. versionadded:: 8.6.0
+    """
+    frame: FrameType | None = sys._getframe(1)
+    level = 1
+
+    while frame is not None:
+        module = frame.f_globals.get("__name__", "")
+
+        if module != "click" and not module.startswith("click."):
+            return level
+
+        frame = frame.f_back
+        level += 1
+
+    return level
+
+
 def _format_deprecated_label(deprecated: bool | str) -> str:
     """Return the parenthesized deprecation label shown in help text."""
     label = _("deprecated").upper()
@@ -2458,6 +2479,43 @@ def _parse_decls(
         self, decls: cabc.Sequence[str], expose_value: bool
     ) -> tuple[str, list[str], list[str]]: ...
 
+    def _check_name_is_usable(self, name: str, decls: cabc.Sequence[str]) -> None:
+        """Warn about a name Click 9.0 will refuse.
+
+        A name is refused for one of two reasons, and never for both: it is not
+        an identifier (``0-file``), or it is a keyword (``from``).
+        ``str.isidentifier`` accepts a keyword, so ``--from`` names a parameter
+        ``from`` today. No callback can declare that, which leaves the value
+        reachable through ``**kwargs`` alone.
+
+        Soft keywords such as ``match`` and ``type`` are contextual and name a
+        parameter fine, so :func:`keyword.iskeyword` passes them.
+
+        Both imports are local because neither :mod:`keyword` nor
+        :mod:`warnings` is on the allow-list ``tests/test_imports.py`` holds
+        Click's import footprint to.
+
+        .. versionadded:: 8.6.0
+        """
+        import keyword
+
+        if keyword.iskeyword(name):
+            reason = "which is a Python keyword"
+        elif not name.isidentifier():
+            reason = "which is not a valid Python identifier"
+        else:
+            return
+
+        import warnings
+
+        warnings.warn(
+            f"{self.param_type_name.capitalize()} {list(decls)!r} uses {name!r}"
+            f" as its name, {reason}. This is deprecated and will raise a"
+            " TypeError in Click 9.0.",
+            DeprecationWarning,
+            stacklevel=_outside_click_stacklevel(),
+        )
+
     @property
     def human_readable_name(self) -> str:
         """Returns the human readable name of this parameter.  This is the
@@ -3274,19 +3332,40 @@ def get_error_hint(self, ctx: Context | None) -> str:
             result += f" (env var: '{self.envvar}')"
         return result
 
+    def _check_name_is_normalized(self, name: str, decls: cabc.Sequence[str]) -> None:
+        """Warn about an explicit name Click 9.0 will spell differently.
+
+        .. versionadded:: 8.6.0
+        """
+        normalized = name.lower()
+
+        if normalized == name:
+            return
+
+        import warnings
+
+        warnings.warn(
+            f"Option {list(decls)!r} uses {name!r} as its name. Click 9.0"
+            f" lower cases an explicit name like any other declaration, naming"
+            f" {normalized!r} instead.",
+            DeprecationWarning,
+            stacklevel=_outside_click_stacklevel(),
+        )
+
     def _parse_decls(
         self, decls: cabc.Sequence[str], expose_value: bool
     ) -> tuple[str, list[str], list[str]]:
         opts = []
         secondary_opts = []
         name = None
+        explicit_name = None
         possible_names = []
 
         for decl in decls:
             if decl.isidentifier():
                 if name is not None:
                     raise TypeError(_("Name '{name}' defined twice").format(name=name))
-                name = decl
+                name = explicit_name = decl
             else:
                 split_char = ";" if decl[:1] == "/" else "/"
                 if split_char in decl:
@@ -3312,12 +3391,12 @@ def _parse_decls(
         if name is None and possible_names:
             possible_names.sort(key=lambda x: -len(x[0]))  # group long options first
             name = possible_names[0][1].replace("-", "_").lower()
-            if not name.isidentifier():
-                name = None
 
-        if name is None:
+        if name is None or not name.isidentifier():
             if not expose_value:
+                self._check_name_is_usable(name or "", decls)
                 return "", opts, secondary_opts
+
             raise TypeError(
                 _(
                     "Could not determine name for option with declarations {decls!r}"
@@ -3333,6 +3412,11 @@ def _parse_decls(
                 ).format(name=name)
             )
 
+        if explicit_name is not None:
+            self._check_name_is_normalized(explicit_name, decls)
+
+        self._check_name_is_usable(name, decls)
+
         return name, opts, secondary_opts
 
     def add_to_parser(self, parser: _OptionParser, ctx: Context) -> None:
@@ -3780,6 +3864,7 @@ def _parse_decls(
     ) -> tuple[str, list[str], list[str]]:
         if not decls:
             if not expose_value:
+                self._check_name_is_usable("", decls)
                 return "", [], []
             raise TypeError("Argument is marked as exposed, but does not have a name.")
         if len(decls) == 1:
@@ -3792,6 +3877,7 @@ def _parse_decls(
                     " {length}: {decls}."
                 ).format(length=len(decls), decls=decls)
             )
+        self._check_name_is_usable(name, decls)
         return name, [arg], []
 
     def get_usage_pieces(self, ctx: Context) -> list[str]: