From 97486c0d987a72a8948075bea3b2821ce3cd3367 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Bern=C3=A1t=20G=C3=A1bor?= Date: Sun, 1 Mar 2026 15:49:22 -0800 Subject: [PATCH 1/2] =?UTF-8?q?=E2=99=BB=EF=B8=8F=20refactor(logic):=20imp?= =?UTF-8?q?rove=20error=20handling=20and=20test=20coverage?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The dynamic import of user-specified modules produced raw Python tracebacks (ImportError, AttributeError) when users misconfigured :module: or :func: directive options. Wrapping these with explicit error messages surfaces the problem clearly in Sphinx's warning output. The module cleanup on the AttributeError path also prevents stale sys.modules entries from causing cross-test contamination when the same generic module name is reused. The duplicated prefix resolution logic between _build_opt_grp_title and _build_sub_cmd_title was fragile to maintain independently. Extracting _resolve_prefix consolidates the branching into one place. Coverage threshold was at 76% while actual coverage sat at ~99.7%, allowing significant regressions to pass unnoticed. Raised to 100% and added tests for previously uncovered argparse features (nargs, choices, count/append actions) and error paths. --- pyproject.toml | 2 +- roots/test-actions/conf.py | 8 +++ roots/test-actions/index.rst | 3 ++ roots/test-actions/parser.py | 11 +++++ roots/test-bad-func/conf.py | 8 +++ roots/test-bad-func/index.rst | 3 ++ roots/test-bad-func/parser.py | 7 +++ roots/test-bad-module/conf.py | 8 +++ roots/test-bad-module/index.rst | 3 ++ roots/test-choices/conf.py | 8 +++ roots/test-choices/index.rst | 3 ++ roots/test-choices/parser.py | 10 ++++ roots/test-nargs/conf.py | 8 +++ roots/test-nargs/index.rst | 3 ++ roots/test-nargs/parser.py | 12 +++++ src/sphinx_argparse_cli/_logic.py | 82 ++++++++++++++++++------------- tests/conftest.py | 4 +- tests/test_logic.py | 37 +++++++++++++- 18 files changed, 181 insertions(+), 39 deletions(-) create mode 100644 roots/test-actions/conf.py create mode 100644 roots/test-actions/index.rst create mode 100644 roots/test-actions/parser.py create mode 100644 roots/test-bad-func/conf.py create mode 100644 roots/test-bad-func/index.rst create mode 100644 roots/test-bad-func/parser.py create mode 100644 roots/test-bad-module/conf.py create mode 100644 roots/test-bad-module/index.rst create mode 100644 roots/test-choices/conf.py create mode 100644 roots/test-choices/index.rst create mode 100644 roots/test-choices/parser.py create mode 100644 roots/test-nargs/conf.py create mode 100644 roots/test-nargs/index.rst create mode 100644 roots/test-nargs/parser.py diff --git a/pyproject.toml b/pyproject.toml index 072aa6f..e07811e 100644 --- a/pyproject.toml +++ b/pyproject.toml @@ -140,7 +140,7 @@ paths.source = [ "src", "**/site-packages", ] -report.fail_under = 76 +report.fail_under = 100 html.show_contexts = true html.skip_covered = false diff --git a/roots/test-actions/conf.py b/roots/test-actions/conf.py new file mode 100644 index 0000000..9f2a54a --- /dev/null +++ b/roots/test-actions/conf.py @@ -0,0 +1,8 @@ +from __future__ import annotations + +import sys +from pathlib import Path + +sys.path.insert(0, str(Path(__file__).parent)) +extensions = ["sphinx_argparse_cli"] +nitpicky = True diff --git a/roots/test-actions/index.rst b/roots/test-actions/index.rst new file mode 100644 index 0000000..708ad9c --- /dev/null +++ b/roots/test-actions/index.rst @@ -0,0 +1,3 @@ +.. sphinx_argparse_cli:: + :module: parser + :func: make diff --git a/roots/test-actions/parser.py b/roots/test-actions/parser.py new file mode 100644 index 0000000..88dc1aa --- /dev/null +++ b/roots/test-actions/parser.py @@ -0,0 +1,11 @@ +from __future__ import annotations + +from argparse import ArgumentParser + + +def make() -> ArgumentParser: + parser = ArgumentParser(prog="actions") + parser.add_argument("-v", "--verbose", action="count", default=0, help="increase verbosity") + parser.add_argument("--include", action="append", help="paths to include") + parser.add_argument("--required-opt", required=True, help="a required optional argument") + return parser diff --git a/roots/test-bad-func/conf.py b/roots/test-bad-func/conf.py new file mode 100644 index 0000000..9f2a54a --- /dev/null +++ b/roots/test-bad-func/conf.py @@ -0,0 +1,8 @@ +from __future__ import annotations + +import sys +from pathlib import Path + +sys.path.insert(0, str(Path(__file__).parent)) +extensions = ["sphinx_argparse_cli"] +nitpicky = True diff --git a/roots/test-bad-func/index.rst b/roots/test-bad-func/index.rst new file mode 100644 index 0000000..272360a --- /dev/null +++ b/roots/test-bad-func/index.rst @@ -0,0 +1,3 @@ +.. sphinx_argparse_cli:: + :module: parser + :func: nonexistent_func diff --git a/roots/test-bad-func/parser.py b/roots/test-bad-func/parser.py new file mode 100644 index 0000000..e0fbafa --- /dev/null +++ b/roots/test-bad-func/parser.py @@ -0,0 +1,7 @@ +from __future__ import annotations + +from argparse import ArgumentParser + + +def make() -> ArgumentParser: + return ArgumentParser(prog="foo") diff --git a/roots/test-bad-module/conf.py b/roots/test-bad-module/conf.py new file mode 100644 index 0000000..9f2a54a --- /dev/null +++ b/roots/test-bad-module/conf.py @@ -0,0 +1,8 @@ +from __future__ import annotations + +import sys +from pathlib import Path + +sys.path.insert(0, str(Path(__file__).parent)) +extensions = ["sphinx_argparse_cli"] +nitpicky = True diff --git a/roots/test-bad-module/index.rst b/roots/test-bad-module/index.rst new file mode 100644 index 0000000..dba2923 --- /dev/null +++ b/roots/test-bad-module/index.rst @@ -0,0 +1,3 @@ +.. sphinx_argparse_cli:: + :module: nonexistent_module + :func: make diff --git a/roots/test-choices/conf.py b/roots/test-choices/conf.py new file mode 100644 index 0000000..9f2a54a --- /dev/null +++ b/roots/test-choices/conf.py @@ -0,0 +1,8 @@ +from __future__ import annotations + +import sys +from pathlib import Path + +sys.path.insert(0, str(Path(__file__).parent)) +extensions = ["sphinx_argparse_cli"] +nitpicky = True diff --git a/roots/test-choices/index.rst b/roots/test-choices/index.rst new file mode 100644 index 0000000..708ad9c --- /dev/null +++ b/roots/test-choices/index.rst @@ -0,0 +1,3 @@ +.. sphinx_argparse_cli:: + :module: parser + :func: make diff --git a/roots/test-choices/parser.py b/roots/test-choices/parser.py new file mode 100644 index 0000000..c2288cb --- /dev/null +++ b/roots/test-choices/parser.py @@ -0,0 +1,10 @@ +from __future__ import annotations + +from argparse import ArgumentParser + + +def make() -> ArgumentParser: + parser = ArgumentParser(prog="choices") + parser.add_argument("--format", choices=["json", "xml", "csv"], help="output format") + parser.add_argument("--level", type=int, choices=[1, 2, 3], help="verbosity level") + return parser diff --git a/roots/test-nargs/conf.py b/roots/test-nargs/conf.py new file mode 100644 index 0000000..9f2a54a --- /dev/null +++ b/roots/test-nargs/conf.py @@ -0,0 +1,8 @@ +from __future__ import annotations + +import sys +from pathlib import Path + +sys.path.insert(0, str(Path(__file__).parent)) +extensions = ["sphinx_argparse_cli"] +nitpicky = True diff --git a/roots/test-nargs/index.rst b/roots/test-nargs/index.rst new file mode 100644 index 0000000..708ad9c --- /dev/null +++ b/roots/test-nargs/index.rst @@ -0,0 +1,3 @@ +.. sphinx_argparse_cli:: + :module: parser + :func: make diff --git a/roots/test-nargs/parser.py b/roots/test-nargs/parser.py new file mode 100644 index 0000000..26dc946 --- /dev/null +++ b/roots/test-nargs/parser.py @@ -0,0 +1,12 @@ +from __future__ import annotations + +from argparse import ArgumentParser + + +def make() -> ArgumentParser: + parser = ArgumentParser(prog="nargs") + parser.add_argument("pos_optional", nargs="?", default="default_val", help="optional positional") + parser.add_argument("pos_zero_or_more", nargs="*", help="zero or more positional") + parser.add_argument("pos_one_or_more", nargs="+", help="one or more positional") + parser.add_argument("--pair", nargs=2, metavar=("KEY", "VALUE"), help="exactly two args") + return parser diff --git a/src/sphinx_argparse_cli/_logic.py b/src/sphinx_argparse_cli/_logic.py index 8d94066..e7e843a 100644 --- a/src/sphinx_argparse_cli/_logic.py +++ b/src/sphinx_argparse_cli/_logic.py @@ -111,7 +111,17 @@ def __init__( # noqa: PLR0913 def parser(self) -> ArgumentParser: if self._parser is None: module_name, attr_name = self.options["module"], self.options["func"] - parser_creator = getattr(__import__(module_name, fromlist=[attr_name]), attr_name) + try: + module = __import__(module_name, fromlist=[attr_name]) + except ImportError: + msg = f"Failed to import module {module_name!r}" + raise self.error(msg) # noqa: B904 + try: + parser_creator = getattr(module, attr_name) + except AttributeError: + del sys.modules[module_name] + msg = f"Module {module_name!r} has no attribute {attr_name!r}" + raise self.error(msg) # noqa: B904 if "hook" in self.options: original_parse_known_args = ArgumentParser.parse_known_args ArgumentParser.parse_known_args = _parse_known_args_hook # type: ignore[method-assign,assignment] @@ -124,7 +134,7 @@ def parser(self) -> ArgumentParser: else: self._parser = parser_creator() - del sys.modules[module_name] # no longer needed cleanup + del sys.modules[module_name] if self._parser is None: msg = "Failed to hook argparse to get ArgumentParser" raise self.error(msg) @@ -174,7 +184,7 @@ def run(self) -> list[Node]: # construct headers self.env.note_reread() # this document needs to be always updated title_text = self.options.get("title", f"{self.parser.prog} - CLI interface").strip() - if not title_text.strip(): + if not title_text: home_section: Element = paragraph() else: home_section = section("", title("", Text(title_text)), ids=[self.make_id(title_text)], names=[title_text]) @@ -236,24 +246,9 @@ def _mk_option_group(self, group: _ArgumentGroup, prefix: str) -> section: return group_section def _build_opt_grp_title(self, group: _ArgumentGroup, prefix: str, sub_title_prefix: str, title_prefix: str) -> str: - title_text, elements = "", prefix.split(" ") - if title_prefix is not None: - title_prefix = title_prefix.replace("{prog}", elements[0]) - if title_prefix: - title_text += f"{title_prefix} " - if " " in prefix: - if sub_title_prefix is not None: - title_text = self._append_title(title_text, sub_title_prefix, elements[0], " ".join(elements[1:])) - else: - title_text += f"{' '.join(prefix.split(' ')[1:])} " - elif " " in prefix: - if sub_title_prefix is not None: - title_text += f"{elements[0]} " - title_text = self._append_title(title_text, sub_title_prefix, elements[0], " ".join(elements[1:])) - else: - title_text += f"{' '.join(elements)} " - else: - title_text += f"{prefix} " + elements = prefix.split(" ") + sub_cmd = " ".join(elements[1:]) if " " in prefix else None + title_text = self._resolve_prefix(elements[0], sub_cmd, prefix, title_prefix, sub_title_prefix) title_text += group.title or "" return title_text @@ -336,11 +331,11 @@ def _mk_sub_command(self, aliases: list[str], help_msg: str, parser: ArgumentPar sub_title_prefix: str = self.options["group_sub_title_prefix"] title_prefix: str = self.options["group_title_prefix"] - if sys.version_info >= (3, 14): + if sys.version_info >= (3, 14): # pragma: >=3.14 cover # https://github.com/python/cpython/issues/139809 parser.prog = _strip_ansi_colors(parser.prog) - title_text = self._build_sub_cmd_title(parser, sub_title_prefix, title_prefix) + title_text = self._build_sub_cmd_title(parser, sub_title_prefix, title_prefix, stripped=True) title_ref: str = parser.prog if aliases: aliases_text: str = f" ({', '.join(aliases)})" @@ -371,26 +366,43 @@ def _mk_sub_command(self, aliases: list[str], help_msg: str, parser: ArgumentPar group_section += self._mk_option_group(group, prefix=parser.prog) return group_section - def _build_sub_cmd_title(self, parser: ArgumentParser, sub_title_prefix: str, title_prefix: str) -> str: - prog = _strip_ansi_colors(parser.prog) - title_text, elements = "", prog.split(" ") + def _build_sub_cmd_title( + self, parser: ArgumentParser, sub_title_prefix: str, title_prefix: str, *, stripped: bool = False + ) -> str: + prog = parser.prog if stripped else _strip_ansi_colors(parser.prog) + elements = prog.split(" ") + return self._resolve_prefix(elements[0], elements[1], prog, title_prefix, sub_title_prefix).rstrip() + + def _resolve_prefix( + self, + prog_name: str, + sub_cmd: str | None, + full_text: str, + title_prefix: str | None, + sub_title_prefix: str | None, + ) -> str: + title_text = "" if title_prefix is not None: - title_prefix = title_prefix.replace("{prog}", elements[0]) + title_prefix = title_prefix.replace("{prog}", prog_name) if title_prefix: title_text += f"{title_prefix} " + if sub_cmd is not None: + if sub_title_prefix is not None: + title_text = self._apply_sub_title(title_text, sub_title_prefix, prog_name, sub_cmd) + else: + title_text += f"{sub_cmd} " + elif sub_cmd is not None: if sub_title_prefix is not None: - title_text = self._append_title(title_text, sub_title_prefix, elements[0], elements[1]) + title_text += f"{prog_name} " + title_text = self._apply_sub_title(title_text, sub_title_prefix, prog_name, sub_cmd) else: - title_text += elements[1] - elif sub_title_prefix is not None: - title_text += f"{elements[0]} " - title_text = self._append_title(title_text, sub_title_prefix, elements[0], elements[1]) + title_text += f"{full_text} " else: - title_text += prog - return title_text.rstrip() + title_text += f"{full_text} " + return title_text @staticmethod - def _append_title(title_text: str, sub_title_prefix: str, prog: str, sub_cmd: str) -> str: + def _apply_sub_title(title_text: str, sub_title_prefix: str, prog: str, sub_cmd: str) -> str: if sub_title_prefix: sub_title_prefix = sub_title_prefix.replace("{prog}", prog) sub_title_prefix = sub_title_prefix.replace("{subcommand}", sub_cmd) diff --git a/tests/conftest.py b/tests/conftest.py index f3f3b5f..ed08907 100644 --- a/tests/conftest.py +++ b/tests/conftest.py @@ -14,7 +14,9 @@ collect_ignore = ["roots"] -def pytest_report_header(config: Config) -> str: # noqa: ARG001 +def pytest_report_header( + config: Config, # noqa: ARG001 +) -> str: # pragma: no cover # runs during collection before coverage starts return f"libraries: Sphinx-{sphinx_version}, docutils-{docutils_version}" diff --git a/tests/test_logic.py b/tests/test_logic.py index 457e60d..168bd5f 100644 --- a/tests/test_logic.py +++ b/tests/test_logic.py @@ -18,8 +18,7 @@ @pytest.fixture(scope="session") def opt_grp_name() -> tuple[str, str]: - return "options", "options" # pragma: no cover - return "optional arguments", "optional-arguments" # pragma: no cover + return "options", "options" @pytest.fixture @@ -348,3 +347,37 @@ def test_subparsers(build_outcome: str) -> None: assert '
' in build_outcome assert '
' in build_outcome assert '
' in build_outcome + + +@pytest.mark.sphinx(buildername="text", testroot="bad-module") +def test_bad_module(app: SphinxTestApp, warning: StringIO) -> None: + app.build() + assert "Failed to import module 'nonexistent_module'" in warning.getvalue() + + +@pytest.mark.sphinx(buildername="text", testroot="bad-func") +def test_bad_func(app: SphinxTestApp, warning: StringIO) -> None: + app.build() + assert "Module 'parser' has no attribute 'nonexistent_func'" in warning.getvalue() + + +@pytest.mark.sphinx(buildername="text", testroot="nargs") +def test_nargs(build_outcome: str) -> None: + assert "pos_optional" in build_outcome + assert "pos_zero_or_more" in build_outcome + assert "pos_one_or_more" in build_outcome + assert "KEY" in build_outcome + assert "VALUE" in build_outcome + + +@pytest.mark.sphinx(buildername="text", testroot="choices") +def test_choices(build_outcome: str) -> None: + assert "output format" in build_outcome + assert "verbosity level" in build_outcome + + +@pytest.mark.sphinx(buildername="text", testroot="actions") +def test_actions(build_outcome: str) -> None: + assert "increase verbosity" in build_outcome + assert "paths to include" in build_outcome + assert "a required optional argument" in build_outcome From b745b17d4b8fbb0b69d389030d5f727e0ece5606 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Bern=C3=A1t=20G=C3=A1bor?= Date: Sun, 1 Mar 2026 18:08:31 -0800 Subject: [PATCH 2/2] =?UTF-8?q?=F0=9F=90=9B=20fix(coverage):=20remove=20de?= =?UTF-8?q?ad=20=5Fstrip=5Fansi=5Fcolors=20path?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit After consolidating the ANSI stripping into _mk_sub_command's version guard, the stripped parameter in _build_sub_cmd_title became dead code — its only caller always passed stripped=True. This left _strip_ansi_colors unreachable on Python <3.14, failing coverage on those versions. --- src/sphinx_argparse_cli/_logic.py | 14 +++++--------- 1 file changed, 5 insertions(+), 9 deletions(-) diff --git a/src/sphinx_argparse_cli/_logic.py b/src/sphinx_argparse_cli/_logic.py index e7e843a..d847ea2 100644 --- a/src/sphinx_argparse_cli/_logic.py +++ b/src/sphinx_argparse_cli/_logic.py @@ -335,7 +335,7 @@ def _mk_sub_command(self, aliases: list[str], help_msg: str, parser: ArgumentPar # https://github.com/python/cpython/issues/139809 parser.prog = _strip_ansi_colors(parser.prog) - title_text = self._build_sub_cmd_title(parser, sub_title_prefix, title_prefix, stripped=True) + title_text = self._build_sub_cmd_title(parser, sub_title_prefix, title_prefix) title_ref: str = parser.prog if aliases: aliases_text: str = f" ({', '.join(aliases)})" @@ -366,12 +366,9 @@ def _mk_sub_command(self, aliases: list[str], help_msg: str, parser: ArgumentPar group_section += self._mk_option_group(group, prefix=parser.prog) return group_section - def _build_sub_cmd_title( - self, parser: ArgumentParser, sub_title_prefix: str, title_prefix: str, *, stripped: bool = False - ) -> str: - prog = parser.prog if stripped else _strip_ansi_colors(parser.prog) - elements = prog.split(" ") - return self._resolve_prefix(elements[0], elements[1], prog, title_prefix, sub_title_prefix).rstrip() + def _build_sub_cmd_title(self, parser: ArgumentParser, sub_title_prefix: str, title_prefix: str) -> str: + elements = parser.prog.split(" ") + return self._resolve_prefix(elements[0], elements[1], parser.prog, title_prefix, sub_title_prefix).rstrip() def _resolve_prefix( self, @@ -445,8 +442,7 @@ def _parse_known_args_hook(self: ArgumentParser, *args: Any, **kwargs: Any) -> N _ANSI_COLOR_RE = re.compile(r"\x1b\[[0-9;]*m") -def _strip_ansi_colors(text: str) -> str: - """Remove ANSI color/style escape sequences (SGR codes) from text.""" +def _strip_ansi_colors(text: str) -> str: # pragma: >=3.14 cover # needed due to https://github.com/python/cpython/issues/139809 return _ANSI_COLOR_RE.sub("", text)