gh-107467: Restructure Argument Clinic command-line interface

- Add and use CLIError exception for CLI usage errors - On CLI error, print to stderr instead of stdout - Put the entire CLI in main() - Rework ClinicExternalTest to call main() instead of using subprocesses
python · erlend-aasland · Aug 1, 2023 · Jul 30, 2023 · Jul 30, 2023 · Jul 30, 2023
commit a16fe3fa2de8b6e551cc891a7a1bad287e75107c
@@ -1391,30 +1391,24 @@ def test_scaffolding(self):
 
 class ClinicExternalTest(TestCase):
     maxDiff = None
-    clinic_py = os.path.join(test_tools.toolsdir, "clinic", "clinic.py")
-
-    def _do_test(self, *args, expect_success=True):
-        with subprocess.Popen(
-            [sys.executable, "-Xutf8", self.clinic_py, *args],
-            encoding="utf-8",
-            bufsize=0,
-            stdout=subprocess.PIPE,
-            stderr=subprocess.PIPE,
-        ) as proc:
-            proc.wait()
-            if expect_success and proc.returncode:
-                self.fail("".join([*proc.stdout, *proc.stderr]))
-            stdout = proc.stdout.read()
-            stderr = proc.stderr.read()
-            # Clinic never writes to stderr.
-            self.assertEqual(stderr, "")
-            return stdout
 
     def expect_success(self, *args):
-        return self._do_test(*args)
+        with support.captured_stdout() as out, support.captured_stderr() as err:
+            try:
+                clinic.main(args)
+            except SystemExit as exc:
+                if exc.code != 0:
+                    self.fail(f"unexpected failure: {args=}")
+        self.assertEqual(err.getvalue(), "")
+        return out.getvalue()
 
     def expect_failure(self, *args):
-        return self._do_test(*args, expect_success=False)
+        with support.captured_stdout() as out, support.captured_stderr() as err:
+            with self.assertRaises(SystemExit) as cm:
+                clinic.main(args)
+            if cm.exception.code == 0:
+                self.fail(f"unexpected success: {args=}")
+        return out.getvalue(), err.getvalue()
 
     def test_external(self):
         CLINIC_TEST = 'clinic.test.c'
@@ -1478,8 +1472,9 @@ def test_cli_force(self):
             # First, run the CLI without -f and expect failure.
             # Note, we cannot check the entire fail msg, because the path to
             # the tmp file will change for every run.
-            out = self.expect_failure(fn)
-            self.assertTrue(out.endswith(fail_msg))
+            out, _ = self.expect_failure(fn)
+            self.assertTrue(out.endswith(fail_msg),
+                            f"{out!r} does not end with {fail_msg!r}")
             # Then, force regeneration; success expected.
             out = self.expect_success("-f", fn)
             self.assertEqual(out, "")
@@ -1621,33 +1616,30 @@ def test_cli_converters(self):
                 )
 
     def test_cli_fail_converters_and_filename(self):
-        out = self.expect_failure("--converters", "test.c")
-        msg = (
-            "Usage error: can't specify --converters "
-            "and a filename at the same time"
-        )
-        self.assertIn(msg, out)
+        _, err = self.expect_failure("--converters", "test.c")
+        msg = "can't specify --converters and a filename at the same time"
+        self.assertIn(msg, err)
 
     def test_cli_fail_no_filename(self):
-        out = self.expect_failure()
-        self.assertIn("usage: clinic.py", out)
+        _, err = self.expect_failure()
+        self.assertIn("no input files", err)
 
     def test_cli_fail_output_and_multiple_files(self):
-        out = self.expect_failure("-o", "out.c", "input.c", "moreinput.c")
+        _, err = self.expect_failure("-o", "out.c", "input.c", "moreinput.c")
         msg = "Usage error: can't use -o with multiple filenames"
-        self.assertIn(msg, out)
+        self.assertIn(msg, err)
 
     def test_cli_fail_filename_or_output_and_make(self):
+        msg = "can't use -o or filenames with --make"
         for opts in ("-o", "out.c"), ("filename.c",):
             with self.subTest(opts=opts):
-                out = self.expect_failure("--make", *opts)
-                msg = "Usage error: can't use -o or filenames with --make"
-                self.assertIn(msg, out)
+                _, err = self.expect_failure("--make", *opts)
+                self.assertIn(msg, err)
 
     def test_cli_fail_make_without_srcdir(self):
-        out = self.expect_failure("--make", "--srcdir", "")
+        _, err = self.expect_failure("--make", "--srcdir", "")
         msg = "Usage error: --srcdir must not be empty with --make"
-        self.assertIn(msg, out)
+        self.assertIn(msg, err)
 
 
 try:

@@ -7,6 +7,7 @@
 from __future__ import annotations
 
 import abc
+import argparse
 import ast
 import builtins as bltns
 import collections
@@ -5620,10 +5621,13 @@ def state_terminal(self, line: str | None) -> None:
 clinic = None
 
 
-def main(argv: list[str]) -> None:
-    import sys
-    import argparse
+class CLIError(Exception):
+    pass
+
+
+def create_cli() -> argparse.ArgumentParser:
     cmdline = argparse.ArgumentParser(
+        prog="clinic.py",
         description="""Preprocessor for CPython C files.
 
 The purpose of the Argument Clinic is automating all the boilerplate involved
@@ -5646,14 +5650,15 @@ def main(argv: list[str]) -> None:
                          help="the directory tree to walk in --make mode")
     cmdline.add_argument("filename", metavar="FILE", type=str, nargs="*",
                          help="the list of files to process")
-    ns = cmdline.parse_args(argv)
+    return cmdline
+
 
+def run_clinic(ns: argparse.Namespace) -> None:
     if ns.converters:
         if ns.filename:
-            print("Usage error: can't specify --converters and a filename at the same time.")
-            print()
-            cmdline.print_usage()
-            sys.exit(-1)
+            raise CLIError(
+                "can't specify --converters and a filename at the same time"
+            )
         converters: list[tuple[str, str]] = []
         return_converters: list[tuple[str, str]] = []
         ignored = set("""
@@ -5711,15 +5716,9 @@ def main(argv: list[str]) -> None:
 
     if ns.make:
         if ns.output or ns.filename:
-            print("Usage error: can't use -o or filenames with --make.")
-            print()
-            cmdline.print_usage()
-            sys.exit(-1)
+            raise CLIError("can't use -o or filenames with --make")
         if not ns.srcdir:
-            print("Usage error: --srcdir must not be empty with --make.")
-            print()
-            cmdline.print_usage()
-            sys.exit(-1)
+            raise CLIError("--srcdir must not be empty with --make")
         for root, dirs, files in os.walk(ns.srcdir):
             for rcs_dir in ('.svn', '.git', '.hg', 'build', 'externals'):
                 if rcs_dir in dirs:
@@ -5735,21 +5734,27 @@ def main(argv: list[str]) -> None:
         return
 
     if not ns.filename:
-        cmdline.print_usage()
-        sys.exit(-1)
+        raise CLIError("no input files")
 
     if ns.output and len(ns.filename) > 1:
-        print("Usage error: can't use -o with multiple filenames.")
-        print()
-        cmdline.print_usage()
-        sys.exit(-1)
+        raise CLIError("can't use -o with multiple filenames")
 
     for filename in ns.filename:
         if ns.verbose:
             print(filename)
         parse_file(filename, output=ns.output, verify=not ns.force)
 
 
+def main(argv: list[str] | None = None) -> None:
+    cli = create_cli()
+    try:
+        args = cli.parse_args(argv)
+        run_clinic(args)
+    except CLIError as exc:
+        if msg := str(exc):
+            sys.stderr.write(f"Usage error: {msg}\n")
+        cli.print_usage()
+        sys.exit(1)
+
 if __name__ == "__main__":
-    main(sys.argv[1:])
-    sys.exit(0)
+    main()