From d8a0c46cb78efedf59182cd95eb1875b6d1f107f Mon Sep 17 00:00:00 2001 From: Kevin Deldycke Date: Tue, 1 Sep 2026 08:44:48 +0200 Subject: [PATCH] Require a parameter name to be a non-keyword Python identifier Make `DeprecationWarning` from #3866 into `TypeError` --- CHANGES.md | 15 +++ docs/arguments.md | 15 +++ docs/options.md | 57 ++++++++--- docs/parameters.md | 115 +++++++++++++++++++++- src/click/core.py | 197 +++++++++++++++---------------------- src/click/parser.py | 20 ++-- tests/test_arguments.py | 113 ++++++++------------- tests/test_deprecations.py | 146 --------------------------- tests/test_options.py | 91 ++++++++++++----- 9 files changed, 385 insertions(+), 384 deletions(-) diff --git a/CHANGES.md b/CHANGES.md index 8853860214..0d26a40351 100644 --- a/CHANGES.md +++ b/CHANGES.md @@ -1,3 +1,18 @@ +## Version 9.0.0 + +Unreleased + +- Breaking: an automatically derived parameter name must be a valid Python + identifier, or `TypeError` is raised. {class}`Argument` and {class}`Option` + derive their name the same way and apply the same check. {pr}`3827` +- Breaking: a parameter name that is a Python keyword raises `TypeError`, where + 8.6 deprecated it. Pass an explicit name instead, as in + `click.option("--from", "source")`. {pr}`3827` {pr}`3866` +- Breaking: `expose_value=False` no longer bypasses the name check, and neither + kind builds a parameter without a declaration. {pr}`3827` +- Breaking: an {class}`Option` now normalizes its declaration written as a Python + identifier to determine the parameter name. {pr}`3827` + ## Version 8.6.0 Unreleased diff --git a/docs/arguments.md b/docs/arguments.md index 14e7556f01..4a5b92fab6 100644 --- a/docs/arguments.md +++ b/docs/arguments.md @@ -51,6 +51,21 @@ recognized, otherwise {data}`STRING` is used. If no default value is provided, the type is assumed to be {data}`STRING`. See {ref}`type-inference` for the types that are recognized. +(argument-names)= + +## Argument Names + +An argument takes exactly one declaration. An option takes several, and +{ref}`picks one of them `. An argument raises {exc}`TypeError` +when it gets more than one declaration. + +Both kinds then derive the name the same way. See the +{ref}`name transform ` for the rules and the examples, +including the {ref}`reserved keywords ` rule. + +The name check also applies when `expose_value=False`. The name is also the key +that the parser uses to store the value. + ```{admonition} Note on Required Arguments :class: note diff --git a/docs/options.md b/docs/options.md index df78abb24e..ff73372e1d 100644 --- a/docs/options.md +++ b/docs/options.md @@ -60,41 +60,66 @@ and converting underscores to dashes. invoke(echo, args=['--string-to-echo', 'Hi!']) ``` -More formally, Click will try to infer the decorated function argument name as -follows: +(option-names)= + +## Option Names + +An option carries several declarations, so Click picks one of them to name the +parameter: 1. If a positional argument is a valid [Python identifier](https://docs.python.org/3/reference/lexical_analysis.html#identifiers) (and thus does not have dashes), it is chosen. 2. If multiple positional arguments are prefixed with `--`, the first one declared is chosen. 3. Otherwise, the first positional argument prefixed with `-` is chosen. -To get the argument name, the chosen positional argument is converted to lower -case, a leading `-` or `--` is removed if found, and any remaining `-` -characters are replaced with `_`. +Click then derives the name from the declaration that it chose, with the +{ref}`name transform `. Rule 1 is no exception. An identifier +declaration says which declaration names the parameter. It does not say how +Click spells the name. + +The name must be one that a callback can declare, because the callback receives +the value as a keyword argument. {ref}`Arguments ` follow the +same rules, including the {ref}`reserved keywords ` rule. ```{eval-rst} .. list-table:: Examples - :widths: 15 15 + :widths: 25 15 15 :header-rows: 1 * - Decorator Arguments - - Inferred Argument Name + - Declaration Chosen + - Inferred Name * - ``"-f", "--foo-bar"`` + - ``--foo-bar`` - foo_bar - * - ``"-x"`` - - x - * - ``"-f", "--filename", "dest"`` - - dest - * - ``"--CamelCase"`` - - camelcase - * - ``"-f", "-fb"`` - - f * - ``"--f", "--foo-bar"`` + - ``--f`` - f + * - ``"-f", "-fb"`` + - ``-f`` + - f + * - ``"-f", "--filename", "dest"`` + - ``dest`` + - dest + * - ``"-f", "--filename", "Dest"`` + - ``Dest`` + - dest * - ``"---f"`` + - ``---f`` - _f + * - ``"---a----b--"`` + - ``---a----b--`` + - _a____b__ ``` +The transform is many-to-one. That is useful here: several options can share +one name to form a [feature switch group](#feature-switch-group). + +The name check also applies when `expose_value=False`. The name is also the key +that the parser uses to store the value, so Click still needs it. Pass an +explicit name instead: +`click.option("--0-file", "zero_file", expose_value=False)`. + ## Basic Example A simple {class}`click.Option` takes one option name. By default, it's assumed @@ -509,6 +534,8 @@ literally. ¹: `default=True` is substituted with `flag_value`. ``` +(feature-switch-group)= + #### Feature switch groups (multiple flags sharing one variable) Several `flag_value` options can target the same parameter name to form a diff --git a/docs/parameters.md b/docs/parameters.md index 58f3219896..ce092d0eb6 100644 --- a/docs/parameters.md +++ b/docs/parameters.md @@ -36,7 +36,7 @@ the Python argument name when calling the decorated function with values. In the example, the argument's name is `filename`. The name must match the python arg name. To provide a different name for use in help text, see {ref}`doc-meta-variables`. -The option's names are `-t` and `--times`. More names are available for options and are covered in {ref}`options`. +The option's names are `-t` and `--times`. More names are available for options and are covered in {ref}`option names `. ```{eval-rst} .. click:example:: @@ -53,3 +53,116 @@ The option's names are `-t` and `--times`. More names are available for options invoke(multi_echo, ['--times=3', 'index.txt'], prog_name='multi_echo') ``` + +(name-transform)= + +Both kinds derive that name the same way. An option drops its prefix first, and +only the leading one or two dashes are ever a prefix. From there every `-` +becomes a `_`, wherever it sits, and the result is lower cased. +It must then be a name a callback can declare, so that it can receive the value +as a keyword argument: an identifier, and not a reserved keyword. {exc}`TypeError` +is raised when it is neither. + +```{eval-rst} +.. list-table:: Examples + :widths: 20 20 15 + :header-rows: 1 + + * - Argument Declaration + - Option Declaration + - Inferred Name + * - ``"foo-bar"`` + - ``"--foo-bar"`` + - foo_bar + * - ``"Foo-Bar"`` + - ``"--Foo-Bar"`` + - foo_bar + * - ``"Foo_Bar"`` + - ``"--Foo_Bar"`` + - foo_bar + * - ``"x"`` + - ``"-x"`` + - x + * - ``"CamelCase"`` + - ``"--CamelCase"`` + - camelcase + * - ``"café"`` + - ``"--café"`` + - café + * - ``"ΟΔΟΣ"`` + - ``"--ΟΔΟΣ"`` + - οδος + * - ``"\N{KELVIN SIGN}"`` + - ``"--\N{KELVIN SIGN}"`` + - k + * - ``"foo-٣"`` + - ``"--foo-٣"`` + - foo_٣ + * - ``"a-----b"`` + - ``"--a-----b"`` + - a_____b + * - ``"a--"`` + - ``"--a--"`` + - a__ + * - ``"-a----b--"`` + - ``"---a----b--"`` + - _a____b__ + * - ``"--"`` + - ``"----"`` + - __ + * - ``"match"`` + - ``"--match"`` + - match + * - ``"type"`` + - ``"--type"`` + - type + * - ``"True"`` + - ``"--True"`` + - true + * - ``"None"`` + - ``"--None"`` + - none + * - ``"0-file"`` + - ``"--0-file"`` + - :exc:`TypeError` + * - ``"٣foo"`` + - ``"--٣foo"`` + - :exc:`TypeError` + * - ``"foo.bar"`` + - ``"--foo.bar"`` + - :exc:`TypeError` + * - ``"foo\N{NON-BREAKING HYPHEN}bar"`` + - ``"--foo\N{NON-BREAKING HYPHEN}bar"`` + - :exc:`TypeError` + * - ``"a\N{ZERO WIDTH SPACE}b"`` + - ``"--a\N{ZERO WIDTH SPACE}b"`` + - :exc:`TypeError` + * - ``""`` + - ``"--"`` + - :exc:`TypeError` + * - ``"from"`` + - ``"--from"`` + - :exc:`TypeError` + * - ``"From"`` + - ``"--From"`` + - :exc:`TypeError` +``` + +The transform is many-to-one and not reversible: the three spellings of +`foo-bar` above all name one parameter. Which declaration is transformed in the +first place is the only thing that differs between the two kinds, covered in +{ref}`option names ` and {ref}`argument names `. + +(keyword-names)= + +```{caution} +A [reserved keyword](https://docs.python.org/3/reference/lexical_analysis.html#keywords) +satisfies {meth}`str.isidentifier`, so a rule of its own refuses it: no callback +can declare a parameter called `from`, so `click.option("--from")` raises +{exc}`TypeError`. + +Pass an explicit name instead: `click.option("--from", "source")`. An argument +takes one declaration and has no explicit-name channel, so rename the +declaration there and pass `metavar` to keep its old display: +`click.argument("source", metavar="FROM")`. +``` diff --git a/src/click/core.py b/src/click/core.py index 1f9323a696..6bb88f8bf7 100644 --- a/src/click/core.py +++ b/src/click/core.py @@ -18,7 +18,6 @@ from gettext import gettext as _ from gettext import ngettext from itertools import repeat -from types import FrameType from types import TracebackType from . import types @@ -105,26 +104,6 @@ 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() @@ -784,16 +763,14 @@ def ensure_object(self, object_type: type[V]) -> V: self.obj = rv = object_type() return rv - def _default_map_has(self, name: str | None) -> bool: + def _default_map_has(self, name: str) -> bool: """Check if :attr:`default_map` contains a real value for ``name``. - Returns ``False`` when the key is absent, the map is ``None``, - ``name`` is ``None``, or the stored value is the internal - :data:`UNSET` sentinel. + Returns ``False`` when the key is absent, the map is ``None``, or the + stored value is the internal :data:`UNSET` sentinel. """ return ( - name is not None - and self.default_map is not None + self.default_map is not None and name in self.default_map and self.default_map[name] is not UNSET ) @@ -2383,9 +2360,7 @@ def __init__( deprecated: bool | str = False, help: str | None = None, ) -> None: - self.name, self.opts, self.secondary_opts = self._parse_decls( - param_decls or (), expose_value - ) + self.name, self.opts, self.secondary_opts = self._parse_decls(param_decls or ()) self.type: types.ParamType[t.Any] = types.convert_type(type, default) # Default nargs to what the type tells us if we have that @@ -2476,44 +2451,71 @@ def __repr__(self) -> str: @abstractmethod def _parse_decls( - self, decls: cabc.Sequence[str], expose_value: bool + self, decls: cabc.Sequence[str] ) -> 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. + @staticmethod + def _name_from_spec(spec: str) -> str: + """Derive a parameter name from a single declaration. + + The declaration is lower-cased and every ``-`` becomes a ``_``, so + ``--input-file``, ``--Input-File`` and ``INPUT_FILE`` all name + ``input_file``. An option passes the declaration with its prefix + already stripped; an argument passes its sole declaration whole. + + The transform is many-to-one, and so cannot be reversed: the name does + not tell you which declaration produced it. + """ + return spec.replace("-", "_").lower() + + def _resolve_name(self, name: str | None, decls: cabc.Sequence[str]) -> str: + """Settle the name derived from ``decls``, or refuse it. + + A parameter's value reaches the command callback as a keyword + argument, so the name has to be one a callback can declare. - 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. + A name is refused for one of two reasons: it is not an identifier + (``0-file``), or it is a keyword (``from``). Soft keywords such as + ``match`` and ``type`` are allowed by :func:`keyword.iskeyword`. - Soft keywords such as ``match`` and ``type`` are contextual and name a - parameter fine, so :func:`keyword.iskeyword` passes them. + ``expose_value=False`` is no exception. The name is also the key the + parser stores the value under, so two parameters that gave it up would + share that key and each read the other's value. - 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. + The :mod:`keyword` import is local because it is not on the allow-list + defined by ``tests/test_imports.py``. - .. versionadded:: 8.6.0 + :raises TypeError: when no name was derived, or the one derived is not + a name a callback can declare. """ import keyword - if keyword.iskeyword(name): - reason = "which is a Python keyword" + if name is None: + message = _( + "{param_type} {decls!r} gave no name for the parameter. Add a" + " valid name to the parameter declaration." + ) elif not name.isidentifier(): - reason = "which is not a valid Python identifier" + message = _( + "{param_type} {decls!r} tried to use {name!r} as its name, but" + " it is not a valid Python identifier. Add a valid name to the" + " parameter declaration." + ) + elif keyword.iskeyword(name): + message = _( + "{param_type} {decls!r} tried to use {name!r} as its name, but" + " it is a Python keyword. Add a valid name to the parameter" + " declaration." + ) else: - return + return name - 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(), + raise TypeError( + message.format( + param_type=self.param_type_name.capitalize(), + decls=list(decls), + name=name, + ) ) @property @@ -3006,6 +3008,9 @@ class Option(Parameter): :param hidden: hide this option from help outputs. :param attrs: Other command arguments described in :class:`Parameter`. + .. versionchanged:: 9.0.0 + An automatic name must be a Python identifier. + .. versionchanged:: 8.4.0 Non-basic ``flag_value`` types (not ``str``, ``int``, ``float``, or ``bool``) are passed through unchanged instead of being stringified. @@ -3085,9 +3090,6 @@ def __init__( # Phase 1: prompt-related attributes. ``_infer_flag_kind`` reads ``self.prompt`` # and ``self.prompt_required`` so this must run first. if prompt is True: - if not self.name: - raise TypeError("'name' is required with 'prompt=True'.") - prompt_text = self.name.replace("_", " ").capitalize() elif prompt is False: prompt_text = None @@ -3332,40 +3334,19 @@ 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 + self, decls: cabc.Sequence[str] ) -> 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 = explicit_name = decl + name = decl else: split_char = ";" if decl[:1] == "/" else "/" if split_char in decl: @@ -3390,18 +3371,14 @@ 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() + name = possible_names[0][1] - if name is None or not name.isidentifier(): - if not expose_value: - self._check_name_is_usable(name or "", decls) - return "", opts, secondary_opts + # Whichever declaration won, it goes through the one transform. A + # declaration written as an identifier is not exempt from it. + if name is not None: + name = self._name_from_spec(name) - raise TypeError( - _( - "Could not determine name for option with declarations {decls!r}" - ).format(decls=decls) - ) + name = self._resolve_name(name, decls) if not opts and not secondary_opts: raise TypeError( @@ -3412,11 +3389,6 @@ 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: @@ -3524,11 +3496,7 @@ def get_help_extra(self, ctx: Context) -> types.OptionHelpExtra: envvar = self.envvar if envvar is None: - if ( - self.allow_from_autoenv - and ctx.auto_envvar_prefix is not None - and self.name - ): + if self.allow_from_autoenv and ctx.auto_envvar_prefix is not None: envvar = f"{ctx.auto_envvar_prefix}_{self.name.upper()}" if envvar is not None: @@ -3666,7 +3634,7 @@ def resolve_envvar_value(self, ctx: Context) -> str | None: if rv is not None: return rv - if self.allow_from_autoenv and ctx.auto_envvar_prefix is not None and self.name: + if self.allow_from_autoenv and ctx.auto_envvar_prefix is not None: envvar = f"{ctx.auto_envvar_prefix}_{self.name.upper()}" rv = os.environ.get(envvar) @@ -3806,6 +3774,11 @@ class Argument(Parameter): :param help: the help string. + .. versionchanged:: 9.0.0 + Exactly one declaration is required, and it must name a Python + identifier once it is lower-cased and every ``-`` is replaced with + ``_``. ``expose_value=False`` is no exception. + .. versionchanged:: 8.5.0 Added the ``help`` parameter. """ @@ -3860,24 +3833,18 @@ def make_metavar(self, ctx: Context) -> str: return var def _parse_decls( - self, decls: cabc.Sequence[str], expose_value: bool + self, decls: cabc.Sequence[str] ) -> 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: - name = arg = decls[0] - name = name.replace("-", "_").lower() - else: + if len(decls) != 1: raise TypeError( _( "Arguments take exactly one parameter declaration, got" " {length}: {decls}." - ).format(length=len(decls), decls=decls) + ).format(length=len(decls), decls=list(decls)) ) - self._check_name_is_usable(name, decls) + + arg = decls[0] + name = self._resolve_name(self._name_from_spec(arg), decls) return name, [arg], [] def get_usage_pieces(self, ctx: Context) -> list[str]: diff --git a/src/click/parser.py b/src/click/parser.py index 4fcbf7caa8..adb323c3df 100644 --- a/src/click/parser.py +++ b/src/click/parser.py @@ -129,7 +129,7 @@ def __init__( self, obj: CoreOption, opts: cabc.Sequence[str], - dest: str | None, + dest: str, action: str | None = None, nargs: int = 1, const: t.Any | None = None, @@ -168,22 +168,22 @@ def takes_value(self) -> bool: def process(self, value: t.Any, state: _ParsingState) -> None: if self.action == "store": - state.opts[self.dest] = value # type: ignore + state.opts[self.dest] = value elif self.action == "store_const": - state.opts[self.dest] = self.const # type: ignore + state.opts[self.dest] = self.const elif self.action == "append": - state.opts.setdefault(self.dest, []).append(value) # type: ignore + state.opts.setdefault(self.dest, []).append(value) elif self.action == "append_const": - state.opts.setdefault(self.dest, []).append(self.const) # type: ignore + state.opts.setdefault(self.dest, []).append(self.const) elif self.action == "count": - state.opts[self.dest] = state.opts.get(self.dest, 0) + 1 # type: ignore + state.opts[self.dest] = state.opts.get(self.dest, 0) + 1 else: raise ValueError(f"unknown action '{self.action}'") state.order.append(self.obj) class _Argument: - def __init__(self, obj: CoreArgument, dest: str | None, nargs: int = 1): + def __init__(self, obj: CoreArgument, dest: str, nargs: int = 1): self.dest = dest self.nargs = nargs self.obj = obj @@ -209,7 +209,7 @@ def process( if value == (): value = UNSET - state.opts[self.dest] = value # type: ignore + state.opts[self.dest] = value state.order.append(self.obj) @@ -266,7 +266,7 @@ def add_option( self, obj: CoreOption, opts: cabc.Sequence[str], - dest: str | None, + dest: str, action: str | None = None, nargs: int = 1, const: t.Any | None = None, @@ -287,7 +287,7 @@ def add_option( for opt in option._long_opts: self._long_opt[opt] = option - def add_argument(self, obj: CoreArgument, dest: str | None, nargs: int = 1) -> None: + def add_argument(self, obj: CoreArgument, dest: str, nargs: int = 1) -> None: """Adds a positional argument named `dest` to the parser. The `obj` can be used to identify the option in the order list diff --git a/tests/test_arguments.py b/tests/test_arguments.py index 75f72889a0..857e9a7692 100644 --- a/tests/test_arguments.py +++ b/tests/test_arguments.py @@ -1,7 +1,6 @@ import itertools import sys import unicodedata -import warnings from unittest import mock import pytest @@ -109,11 +108,8 @@ def cmd(**kwargs): def test_argument_normalizes_an_identifier_decl(): - """An argument transforms its declaration; an option keeps an explicit name.""" assert click.Argument(["Foo_Bar"]).name == "foo_bar" - - with pytest.warns(DeprecationWarning, match="lower cases an explicit name"): - assert click.Option(["--x", "Foo_Bar"]).name == "Foo_Bar" + assert click.Option(["--x", "Foo_Bar"]).name == "foo_bar" PARAM_KINDS = [ @@ -131,16 +127,6 @@ def test_argument_normalizes_an_identifier_decl(): pytest.param("foo-٣", "foo_٣", id="arabic-indic-digit"), ] -NAME_TRANSFORM_DECLS = UNICODE_CASE_DECLS + [ - pytest.param("a-b-c", "a_b_c", id="interior-singles"), - pytest.param("a-----b", "a_____b", id="interior-run"), - pytest.param("a--", "a__", id="trailing"), - pytest.param("-a-", "_a_", id="leading-survives-the-prefix"), - pytest.param("-a----b--", "_a____b__", id="everywhere"), - pytest.param("--", "__", id="dashes-only"), - pytest.param("_-_", "___", id="around-an-underscore"), -] - # Declarations covering every shape the naming transform accepts. NAME_SWEEP_DECLS = [ "", @@ -163,45 +149,37 @@ def test_argument_normalizes_an_identifier_decl(): @pytest.mark.parametrize("count", [1, 2]) @pytest.mark.parametrize("expose_value", [True, False]) -def test_parameter_name_is_an_identifier_or_deprecated(count, expose_value): +def test_parameter_name_is_always_an_identifier(count, expose_value): built = 0 for decls in itertools.product(NAME_SWEEP_DECLS, repeat=count): for cls in (click.Option, click.Argument): - with warnings.catch_warnings(record=True) as caught: - warnings.simplefilter("always") - - try: - param = cls(list(decls), expose_value=expose_value) - except (TypeError, ValueError): - continue - - built += 1 - - if param.name.isidentifier(): + try: + param = cls(list(decls), expose_value=expose_value) + except (TypeError, ValueError): continue - assert any(issubclass(w.category, DeprecationWarning) for w in caught), ( + built += 1 + assert param.name.isidentifier(), ( f"{cls.__name__}({list(decls)!r}, expose_value={expose_value})" - f" named its parameter {param.name!r} without a warning" + f" named its parameter {param.name!r}" ) assert built, "the sweep built no parameter, so it proves nothing" def test_argument_requires_its_one_declaration(): - """An exposed argument with no declaration is refused, an unexposed one warns.""" - with pytest.raises(TypeError, match="does not have a name"): + """An argument with no declaration is refused, whatever ``expose_value`` says.""" + with pytest.raises(TypeError, match="exactly one parameter declaration"): click.Argument([]) - with pytest.warns(DeprecationWarning, match="not a valid Python identifier"): - assert click.Argument([], expose_value=False).name == "" + with pytest.raises(TypeError, match="exactly one parameter declaration"): + click.Argument([], expose_value=False) def test_argument_name_check_applies_when_not_exposed(): - """An unexposed argument keeps the name, and says Click 9.0 will refuse it.""" - with pytest.warns(DeprecationWarning, match="not a valid Python identifier"): - assert click.Argument(["0foo"], expose_value=False).name == "0foo" + with pytest.raises(TypeError, match="valid Python identifier"): + click.Argument(["0foo"], expose_value=False) def test_argument_metavar_renders_what_a_declaration_may_not(runner): @@ -233,25 +211,38 @@ def cmd(**kwargs): @pytest.mark.parametrize(("cls", "form"), PARAM_KINDS) -@pytest.mark.parametrize(("decl", "expect"), NAME_TRANSFORM_DECLS) -def test_parameter_name_transform(cls, form, decl, expect): - """``str.lower()`` is neither one-to-one nor length-preserving, and a - dash becomes an underscore everywhere except an option's prefix. - """ +@pytest.mark.parametrize(("decl", "expect"), UNICODE_CASE_DECLS) +def test_parameter_name_unicode_case_transform(cls, form, decl, expect): + """``str.lower()`` is neither one-to-one nor length-preserving.""" assert cls([form.format(decl=decl)]).name == expect @pytest.mark.parametrize(("decl", "expect"), UNICODE_CASE_DECLS) -def test_option_explicit_name_keeps_its_case(decl, expect): - """An explicit name is kept as spelled, where Click 9.0 transforms it.""" +def test_option_explicit_name_runs_the_same_case_transform(decl, expect): + """An explicit name is transformed exactly as a derived one is.""" if not decl.isidentifier(): assert click.Option(["--x", decl]).name == "x" return - with pytest.warns(DeprecationWarning, match="lower cases an explicit name"): - assert click.Option(["--x", decl]).name == decl + assert click.Option(["--x", decl]).name == expect - assert decl.lower() == expect + +@pytest.mark.parametrize( + ("arg_decl", "opt_decl", "expect"), + [ + pytest.param("a-b-c", "--a-b-c", "a_b_c", id="interior-singles"), + pytest.param("a-----b", "--a-----b", "a_____b", id="interior-run"), + pytest.param("a--", "--a--", "a__", id="trailing"), + pytest.param("-a-", "---a-", "_a_", id="leading-survives-the-prefix"), + pytest.param("-a----b--", "---a----b--", "_a____b__", id="everywhere"), + pytest.param("--", "----", "__", id="dashes-only"), + pytest.param("_-_", "--_-_", "___", id="around-an-underscore"), + ], +) +def test_parameter_name_keeps_every_dash_past_the_prefix(arg_decl, opt_decl, expect): + """Only an option's leading dashes are considered a prefix.""" + assert click.Argument([arg_decl]).name == expect + assert click.Option([opt_decl], is_flag=True).name == expect @pytest.mark.parametrize(("cls", "form"), PARAM_KINDS) @@ -275,22 +266,9 @@ def test_option_explicit_name_keeps_its_case(decl, expect): pytest.param("", id="empty"), ], ) -def test_parameter_name_not_an_identifier_is_deprecated(cls, form, decl): - """Click 9.0 refuses every one of these; 8.6 keeps them and says so. - - An exposed option is the exception, since it already refuses them. - """ - spec = form.format(decl=decl) - - if cls is click.Option: - with pytest.raises(TypeError, match="Could not determine name"): - cls([spec]) - return - - with pytest.warns(DeprecationWarning, match="not a valid Python identifier"): - param = cls([spec]) - - assert not param.name.isidentifier() +def test_parameter_name_must_be_an_identifier(cls, form, decl): + with pytest.raises(TypeError, match="valid Python identifier"): + cls([form.format(decl=decl)]) @pytest.mark.parametrize(("cls", "form"), PARAM_KINDS) @@ -307,17 +285,12 @@ def test_parameter_name_identifier_check_follows_the_unicode_table(cls, form, ch assert name.isidentifier() == (sys.version_info >= (3, 13)) decl = form.format(decl=name) - if name.isidentifier(): - assert cls([decl]).name == name - return - - if cls is click.Option: - with pytest.raises(TypeError, match="Could not determine name"): + if not name.isidentifier(): + with pytest.raises(TypeError, match="valid Python identifier"): cls([decl]) return - with pytest.warns(DeprecationWarning, match="not a valid Python identifier"): - assert cls([decl]).name == name + assert cls([decl]).name == name @pytest.mark.parametrize( diff --git a/tests/test_deprecations.py b/tests/test_deprecations.py index 49e709a44a..25e44f3c10 100644 --- a/tests/test_deprecations.py +++ b/tests/test_deprecations.py @@ -1,5 +1,4 @@ import importlib.metadata -import warnings import pytest @@ -72,148 +71,3 @@ def test_isolated_filesystem_deprecated(runner): with pytest.warns(DeprecationWarning, match="isolated_filesystem"): with runner.isolated_filesystem(): pass - - -@pytest.mark.parametrize( - ("make_param", "match"), - [ - pytest.param( - lambda: click.Argument(["0foo"]), - "not a valid Python identifier", - id="argument-leading-digit", - ), - pytest.param( - lambda: click.Argument(["foo.bar"]), - "not a valid Python identifier", - id="argument-dot", - ), - pytest.param( - lambda: click.Argument(["foo bar"]), - "not a valid Python identifier", - id="argument-space", - ), - pytest.param( - lambda: click.Argument([""]), - "not a valid Python identifier", - id="argument-empty-decl", - ), - pytest.param( - lambda: click.Argument(["0foo"], expose_value=False), - "not a valid Python identifier", - id="argument-unexposed", - ), - pytest.param( - lambda: click.Argument([], expose_value=False), - "not a valid Python identifier", - id="argument-no-decl", - ), - pytest.param( - lambda: click.Option(["--0foo"], expose_value=False), - "not a valid Python identifier", - id="option-unexposed", - ), - pytest.param( - lambda: click.Option([], expose_value=False), - "not a valid Python identifier", - id="option-no-decl", - ), - # A keyword is an identifier too, so this rule alone reports it. - pytest.param( - lambda: click.Option(["--from"]), - "is a Python keyword", - id="option-from", - ), - pytest.param( - lambda: click.Option(["--import"]), - "is a Python keyword", - id="option-import", - ), - pytest.param( - lambda: click.Argument(["class"]), - "is a Python keyword", - id="argument-class", - ), - ], -) -def test_unusable_name_deprecated(make_param, match): - """A name Click 9.0 refuses reports which of its two rules it broke.""" - with pytest.warns(DeprecationWarning, match=match): - make_param() - - -@pytest.mark.parametrize( - "make_param", - [ - pytest.param(lambda: click.Argument(["foo-bar"]), id="argument-hyphen"), - pytest.param(lambda: click.Argument(["Foo_Bar"]), id="argument-case"), - pytest.param(lambda: click.Option(["--foo-bar"]), id="option-hyphen"), - pytest.param( - lambda: click.Option(["--0foo", "zero_foo"]), id="option-explicit-name" - ), - # Soft keywords are contextual and name a parameter fine. - pytest.param(lambda: click.Option(["--match"]), id="soft-keyword-match"), - pytest.param(lambda: click.Argument(["type"]), id="soft-keyword-type"), - # These lower case out of the keyword set. - pytest.param(lambda: click.Option(["--True"]), id="true"), - pytest.param(lambda: click.Option(["--None"]), id="none"), - ], -) -def test_usable_name_not_deprecated(recwarn, make_param): - """A name a callback can declare keeps working, and says nothing.""" - make_param() - assert [w for w in recwarn if issubclass(w.category, DeprecationWarning)] == [] - - -@pytest.mark.parametrize( - "make_param", - [ - pytest.param(lambda: click.Option(["--from"]), id="keyword"), - pytest.param(lambda: click.Argument(["0foo"]), id="non-identifier"), - ], -) -def test_unusable_name_names_the_release_that_refuses_it(make_param): - """Both refusals name the release that turns the warning into an error.""" - match = r"will raise a TypeError in Click 9\.0" - with pytest.warns(DeprecationWarning, match=match): - make_param() - - -@pytest.mark.parametrize( - ("decl", "expect"), - [ - pytest.param("Foo_Bar", "foo_bar", id="mixed-case"), - pytest.param("X_Y", "x_y", id="upper-case"), - pytest.param("ΟΔΟΣ", "οδος", id="final-sigma"), - pytest.param("\N{KELVIN SIGN}", "k", id="kelvin-sign"), - ], -) -def test_unnormalized_explicit_name_deprecated(decl, expect): - """Click 9.0 lower cases an explicit name like any other declaration.""" - with pytest.warns(DeprecationWarning, match="lower cases an explicit name"): - param = click.Option(["--x", decl]) - - assert param.name == decl - assert decl.lower() == expect - - -@pytest.mark.parametrize( - "decls", - [ - pytest.param(["--x", "foo_bar"], id="already-lower"), - pytest.param(["--x", "_from"], id="leading-underscore"), - pytest.param(["--Foo-Bar"], id="derived-not-explicit"), - pytest.param(["--x", "café"], id="lower-case-non-ascii"), - ], -) -def test_normalized_explicit_name_not_deprecated(recwarn, decls): - """A name already spelled the way 9.0 spells it says nothing.""" - click.Option(decls) - assert [w for w in recwarn if issubclass(w.category, DeprecationWarning)] == [] - - -def test_unnormalized_name_on_a_refused_option_is_silent(): - """An option that cannot be built at all warns about nothing.""" - with pytest.raises(TypeError, match="No options defined"): - with warnings.catch_warnings(): - warnings.simplefilter("error", DeprecationWarning) - click.Option(["Foo_Bar"]) diff --git a/tests/test_options.py b/tests/test_options.py index 472ec1cc60..353fc23cef 100644 --- a/tests/test_options.py +++ b/tests/test_options.py @@ -873,22 +873,21 @@ def cmd(flag): def test_auto_envvar_flattens_name_case(runner): - """Two names differing only by case share one auto envvar.""" - with pytest.warns(DeprecationWarning, match="lower cases an explicit name"): + """Declarations differing only by case name one parameter, and one envvar.""" - @click.command() - @click.option("--foo-bar") - @click.option("--other", "Foo_Bar") - def cmd(**kwargs): - click.echo(repr(sorted(kwargs.items()))) + @click.command() + @click.option("--foo-bar") + @click.option("--other", "Foo_Bar") + def cmd(**kwargs): + click.echo(repr(sorted(kwargs.items()))) - assert [p.name for p in cmd.params if p.name] == ["foo_bar", "Foo_Bar"] + assert [p.name for p in cmd.params if p.name] == ["foo_bar", "foo_bar"] result = runner.invoke( cmd, [], auto_envvar_prefix="TEST", env={"TEST_FOO_BAR": "foo"} ) assert not result.exception - assert result.output == "[('Foo_Bar', 'foo'), ('foo_bar', 'foo')]\n" + assert result.output == "[('foo_bar', 'foo')]\n" def test_auto_envvar_upper_can_change_length(runner): @@ -1406,6 +1405,7 @@ def cli_alt(warnings): (["--FOO-BAR", "-F"], "foo_bar"), # An identifier declaration goes through the same transform. (["--foo-bar", "-f", "explicit_name"], "explicit_name"), + (["--foo-bar", "-f", "Explicit_Name"], "explicit_name"), # Underscores survive, and every dash past the prefix becomes one. (["--foo__bar"], "foo__bar"), (["--foo--bar"], "foo__bar"), @@ -1497,21 +1497,19 @@ def cmd(**kwargs): def test_option_name_must_be_an_identifier(): """A short option is the one refused shape an argument cannot be written as.""" - with pytest.raises(TypeError, match="Could not determine name"): + with pytest.raises(TypeError, match="valid Python identifier"): click.Option(["-0"]) -def test_option_prompt_still_guards_against_a_nameless_option(): - """An unexposed option can still be named ``""``, which ``prompt=True`` refuses.""" - with pytest.warns(DeprecationWarning, match="not a valid Python identifier"): - with pytest.raises(TypeError, match="'name' is required with 'prompt=True'"): - click.Option(["--0-file"], expose_value=False, prompt=True) +def test_option_prompt_needs_no_name_guard(): + """``prompt=True`` is refused by naming first, so it needs no guard of its own.""" + with pytest.raises(TypeError, match="valid Python identifier"): + click.Option(["--0-file"], expose_value=False, prompt=True) def test_option_name_check_applies_when_not_exposed(): - """An unexposed option is named ``""``, and says Click 9.0 will refuse it.""" - with pytest.warns(DeprecationWarning, match="not a valid Python identifier"): - assert click.Option(["--0foo"], expose_value=False).name == "" + with pytest.raises(TypeError, match="valid Python identifier"): + click.Option(["--0foo"], expose_value=False) def test_option_explicit_name_carries_a_refused_declaration(runner): @@ -1534,20 +1532,59 @@ def cmd(**kwargs): assert seen == ["value"] -def test_option_name_may_be_a_python_keyword(runner): - """``str.isidentifier()`` accepts a keyword, so the check lets one through.""" - with pytest.warns(DeprecationWarning, match="is a Python keyword"): +def test_option_needs_at_least_one_option_declaration(): + """A lone identifier names the parameter, but declares no option to parse.""" + with pytest.raises(TypeError, match="No options defined"): + click.Option(["Foo_Bar"]) - @click.command() - @click.option("--from") - def cmd(**kwargs): - click.echo(repr(kwargs)) - assert cmd.params[0].name == "from" +@pytest.mark.parametrize( + ("cls", "decl"), + [ + pytest.param(click.Option, "--from", id="option-from"), + pytest.param(click.Option, "--import", id="option-import"), + pytest.param(click.Argument, "class", id="argument-class"), + # A declaration lower cases into the keyword set on its way to a name. + pytest.param(click.Option, "--From", id="option-mixed-case"), + pytest.param(click.Argument, "Class", id="argument-mixed-case"), + ], +) +def test_parameter_name_may_not_be_a_python_keyword(cls, decl): + """``str.isidentifier()`` accepts a keyword, so a rule of its own refuses it.""" + with pytest.raises(TypeError, match="is a Python keyword"): + cls([decl]) + + +@pytest.mark.parametrize( + ("cls", "decl", "expect"), + [ + # Soft keywords are contextual and name a parameter fine. + pytest.param(click.Option, "--match", "match", id="soft-keyword-match"), + pytest.param(click.Argument, "type", "type", id="soft-keyword-type"), + # These lower case out of the keyword set. + pytest.param(click.Option, "--True", "true", id="true"), + pytest.param(click.Option, "--None", "none", id="none"), + # A keyword with anything attached to it is not one. + pytest.param(click.Argument, "_from", "_from", id="leading-underscore"), + ], +) +def test_parameter_name_near_a_python_keyword_is_accepted(cls, decl, expect): + assert cls([decl]).name == expect + + +def test_option_keyword_name_is_reached_through_an_explicit_name(runner): + """An explicit name carries a declaration whose own name would be refused.""" + + @click.command() + @click.option("--from", "source") + def cmd(**kwargs): + click.echo(repr(kwargs)) + + assert cmd.params[0].name == "source" result = runner.invoke(cmd, ["--from", "here"]) assert not result.exception - assert result.output == "{'from': 'here'}\n" + assert result.output == "{'source': 'here'}\n" def test_flag_duplicate_names(runner):