config: make Argument a wrapper around argparse.Action

Same as in `OptionGroup`, this continues making our Parser code just a
simple wrapper around argparse.

Author: bluetech

src/_pytest/config/__init__.py

@@ -1208,12 +1208,11 @@ def fromdictargs(cls, option_dict: Mapping[str, Any], args: list[str]) -> Config
         return config
 
     def _processopt(self, opt: Argument) -> None:
-        for name in opt._short_opts + opt._long_opts:
+        for name in opt.names():
             self._opt2dest[name] = opt.dest
 
-        if hasattr(opt, "default"):
-            if not hasattr(self.option, opt.dest):
-                setattr(self.option, opt.dest, opt.default)
+        if not hasattr(self.option, opt.dest):
+            setattr(self.option, opt.dest, opt.default)
 
     @hookimpl(trylast=True)
     def pytest_load_initial_conftests(self, early_config: Config) -> None:

src/_pytest/config/argparsing.py

@@ -3,7 +3,6 @@
 
 import argparse
 from collections.abc import Callable
-from collections.abc import Mapping
 from collections.abc import Sequence
 import os
 import sys
@@ -280,103 +279,37 @@ def get_ini_default_for_type(
         return ""
 
 
-class ArgumentError(Exception):
-    """Raised if an Argument instance is created with invalid or
-    inconsistent arguments."""
-
-    def __init__(self, msg: str, option: Argument | str) -> None:
-        self.msg = msg
-        self.option_id = str(option)
-
-    def __str__(self) -> str:
-        if self.option_id:
-            return f"option {self.option_id}: {self.msg}"
-        else:
-            return self.msg
-
-
 class Argument:
-    """Class that mimics the necessary behaviour of optparse.Option.
+    """An option defined in an OptionGroup."""
 
-    It's currently a least effort implementation and ignoring choices
-    and integer prefixes.
+    def __init__(self, action: argparse.Action) -> None:
+        self._action = action
 
-    https://docs.python.org/3/library/optparse.html#optparse-standard-option-types
-    """
+    def attrs(self) -> dict[str, Any]:
+        return self._action.__dict__
 
-    def __init__(self, *names: str, **attrs: Any) -> None:
-        """Store params in private vars for use in add_argument."""
-        self._attrs = attrs
-        self._short_opts: list[str] = []
-        self._long_opts: list[str] = []
-        try:
-            self.type = attrs["type"]
-        except KeyError:
-            pass
-        try:
-            # Attribute existence is tested in Config._processopt.
-            self.default = attrs["default"]
-        except KeyError:
-            pass
-        self._set_opt_strings(names)
-        dest: str | None = attrs.get("dest")
-        if dest:
-            self.dest = dest
-        elif self._long_opts:
-            self.dest = self._long_opts[0][2:].replace("-", "_")
-        else:
-            try:
-                self.dest = self._short_opts[0][1:]
-            except IndexError as e:
-                self.dest = "???"  # Needed for the error repr.
-                raise ArgumentError("need a long or short option", self) from e
+    def names(self) -> Sequence[str]:
+        return self._action.option_strings
 
-    def names(self) -> list[str]:
-        return self._short_opts + self._long_opts
-
-    def attrs(self) -> Mapping[str, Any]:
-        return self._attrs
+    @property
+    def dest(self) -> str:
+        return self._action.dest
 
-    def _set_opt_strings(self, opts: Sequence[str]) -> None:
-        """Directly from optparse.
+    @property
+    def default(self) -> Any:
+        return self._action.default
 
-        Might not be necessary as this is passed to argparse later on.
-        """
-        for opt in opts:
-            if len(opt) < 2:
-                raise ArgumentError(
-                    f"invalid option string {opt!r}: "
-                    "must be at least two characters long",
-                    self,
-                )
-            elif len(opt) == 2:
-                if not (opt[0] == "-" and opt[1] != "-"):
-                    raise ArgumentError(
-                        f"invalid short option string {opt!r}: "
-                        "must be of the form -x, (x any non-dash char)",
-                        self,
-                    )
-                self._short_opts.append(opt)
-            else:
-                if not (opt[0:2] == "--" and opt[2] != "-"):
-                    raise ArgumentError(
-                        f"invalid long option string {opt!r}: "
-                        "must start with --, followed by non-dash",
-                        self,
-                    )
-                self._long_opts.append(opt)
+    @property
+    def type(self) -> Any | None:
+        return self._action.type
 
     def __repr__(self) -> str:
         args: list[str] = []
-        if self._short_opts:
-            args += ["_short_opts: " + repr(self._short_opts)]
-        if self._long_opts:
-            args += ["_long_opts: " + repr(self._long_opts)]
+        args += ["opts: " + repr(self.names())]
         args += ["dest: " + repr(self.dest)]
-        if hasattr(self, "type"):
+        if self._action.type:
             args += ["type: " + repr(self.type)]
-        if hasattr(self, "default"):
-            args += ["default: " + repr(self.default)]
+        args += ["default: " + repr(self.default)]
         return "Argument({})".format(", ".join(args))
 
 
@@ -406,6 +339,7 @@ def addoption(self, *opts: str, **attrs: Any) -> None:
 
         :param opts:
             Option names, can be short or long options.
+            Note that lower-case short options (e.g. `-x`) are reserved.
         :param attrs:
             Same attributes as the argparse library's :meth:`add_argument()
             <argparse.ArgumentParser.add_argument>` function accepts.
@@ -415,25 +349,27 @@ def addoption(self, *opts: str, **attrs: Any) -> None:
         )
         if conflict:
             raise ValueError(f"option names {conflict} already added")
-        option = Argument(*opts, **attrs)
-        self._addoption_instance(option, shortupper=False)
+        self._addoption_inner(opts, attrs, allow_reserved=False)
 
     def _addoption(self, *opts: str, **attrs: Any) -> None:
-        option = Argument(*opts, **attrs)
-        self._addoption_instance(option, shortupper=True)
+        """Like addoption(), but also allows registering short lower case options (e.g. -x),
+        which are reserved for pytest core."""
+        self._addoption_inner(opts, attrs, allow_reserved=True)
 
-    def _addoption_instance(self, option: Argument, shortupper: bool = False) -> None:
-        if not shortupper:
-            for opt in option._short_opts:
-                if opt[0] == "-" and opt[1].islower():
-                    raise ValueError("lowercase shortoptions reserved")
+    def _addoption_inner(
+        self, opts: tuple[str, ...], attrs: dict[str, Any], allow_reserved: bool
+    ) -> None:
+        if not allow_reserved:
+            for opt in opts:
+                if len(opt) >= 2 and opt[0] == "-" and opt[1].islower():
+                    raise ValueError("lowercase short options are reserved")
 
+        action = self._arggroup.add_argument(*opts, **attrs)
+        option = Argument(action)
+        self.options.append(option)
         if self.parser:
             self.parser.processoption(option)
 
-        self._arggroup.add_argument(*option.names(), **option.attrs())
-        self.options.append(option)
-
 
 class PytestArgumentParser(argparse.ArgumentParser):
     def __init__(
@@ -495,10 +431,9 @@ def _format_action_invocation(self, action: argparse.Action) -> str:
         for option in options:
             if len(option) == 2 or option[2] == " ":
                 continue
-            if not option.startswith("--"):
-                raise ArgumentError(
-                    f'long optional argument without "--": [{option}]', option
-                )
+            assert option.startswith("--"), (
+                f'long optional argument without "--": [{option}]'
+            )
             xxoption = option[2:]
             shortened = xxoption.replace("-", "")
             if shortened not in short_long or len(short_long[shortened]) < len(

src/_pytest/helpconfig.py

@@ -59,8 +59,8 @@ def __call__(
 def pytest_addoption(parser: Parser) -> None:
     group = parser.getgroup("debugconfig")
     group.addoption(
-        "--version",
         "-V",
+        "--version",
         action="count",
         default=0,
         dest="version",

testing/test_parseopt.py

@@ -34,33 +34,43 @@ def test_custom_prog(self, parser: parseopt.Parser) -> None:
         assert parser.optparser.prog == "custom-prog"
 
     def test_argument(self) -> None:
-        with pytest.raises(parseopt.ArgumentError):
-            # need a short or long option
-            argument = parseopt.Argument()
-        argument = parseopt.Argument("-t")
-        assert argument._short_opts == ["-t"]
-        assert argument._long_opts == []
-        assert argument.dest == "t"
-        argument = parseopt.Argument("-t", "--test")
-        assert argument._short_opts == ["-t"]
-        assert argument._long_opts == ["--test"]
-        assert argument.dest == "test"
-        argument = parseopt.Argument("-t", "--test", dest="abc")
+        parser = argparse.ArgumentParser()
+
+        action = parser.add_argument("-a")
+        argument = parseopt.Argument(action)
+        assert argument.names() == ["-a"]
+        assert argument.dest == "a"
+
+        action = parser.add_argument("-b", "--boop")
+        argument = parseopt.Argument(action)
+        assert argument.names() == ["-b", "--boop"]
+        assert argument.dest == "boop"
+
+        action = parser.add_argument("-c", "--coop", dest="abc")
+        argument = parseopt.Argument(action)
         assert argument.dest == "abc"
-        assert str(argument) == (
-            "Argument(_short_opts: ['-t'], _long_opts: ['--test'], dest: 'abc')"
+        assert (
+            str(argument)
+            == "Argument(opts: ['-c', '--coop'], dest: 'abc', default: None)"
         )
 
     def test_argument_type(self) -> None:
-        argument = parseopt.Argument("-t", dest="abc", type=int)
+        parser = argparse.ArgumentParser()
+
+        action = parser.add_argument("-a", dest="aa", type=int)
+        argument = parseopt.Argument(action)
         assert argument.type is int
-        argument = parseopt.Argument("-t", dest="abc", type=str)
+
+        action = parser.add_argument("-b", dest="bb", type=str)
+        argument = parseopt.Argument(action)
         assert argument.type is str
-        argument = parseopt.Argument("-t", dest="abc", type=float)
+
+        action = parser.add_argument("-c", dest="cc", type=float)
+        argument = parseopt.Argument(action)
         assert argument.type is float
-        argument = parseopt.Argument(
-            "-t", dest="abc", type=str, choices=["red", "blue"]
-        )
+
+        action = parser.add_argument("-d", dest="dd", type=str, choices=["red", "blue"])
+        argument = parseopt.Argument(action)
         assert argument.type is str
 
     def test_group_add_and_get(self, parser: parseopt.Parser) -> None: