♻️ refactor(logic): improve error handling and reduce duplication (#286)

gaborbernat · web-flow · commit 0790dd22a192 · 2026-03-01T19:35:57.000-08:00
diff --git 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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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,7 +331,7 @@ 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)
 
@@ -372,25 +367,39 @@ def _mk_sub_command(self, aliases: list[str], help_msg: str, parser: ArgumentPar
         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(" ")
+        elements = parser.prog.split(" ")
+        return self._resolve_prefix(elements[0], elements[1], parser.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)
@@ -433,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)
 
diff --git 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
@@ -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 '<section id="test-no_child">' in build_outcome
     assert '<section id="test-no_child-positional-arguments">' in build_outcome
     assert '<section id="test-no_child-options">' 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

Original file line number	Diff line number	Diff line change
`@@ -140,7 +140,7 @@ paths.source = [`
`140`	`140`	`"src",`
`141`	`141`	`"**/site-packages",`
`142`	`142`	`]`
`143`		`-report.fail_under = 76`
	`143`	`+report.fail_under = 100`
`144`	`144`	`html.show_contexts = true`
`145`	`145`	`html.skip_covered = false`
`146`	`146`
Original file line number	Diff line number	Diff line change
`@@ -0,0 +1,3 @@`
	`1`	`+.. sphinx_argparse_cli::`
	`2`	`+ :module: parser`
	`3`	`+ :func: make`
Original file line number	Diff line number	Diff line change
`@@ -0,0 +1,3 @@`
	`1`	`+.. sphinx_argparse_cli::`
	`2`	`+ :module: nonexistent_module`
	`3`	`+ :func: make`