From 49f238e78c36532bcbca7f9cd172703eb4df319b Mon Sep 17 00:00:00 2001 From: "Erlend E. Aasland" Date: Tue, 1 Aug 2023 18:24:23 +0200 Subject: [PATCH] gh-107467: Restructure Argument Clinic command-line interface (#107469) - Use ArgumentParser.error() to handle CLI errors - Put the entire CLI in main() - Rework ClinicExternalTest to call main() instead of using subprocesses Co-authored-by: AlexWaygood --- Lib/test/test_clinic.py | 74 +++++++++---------- ...-07-30-23-32-16.gh-issue-107467.5O9p3G.rst | 2 + Tools/clinic/clinic.py | 47 ++++++------ 3 files changed, 57 insertions(+), 66 deletions(-) create mode 100644 Misc/NEWS.d/next/Tools-Demos/2023-07-30-23-32-16.gh-issue-107467.5O9p3G.rst diff --git a/Lib/test/test_clinic.py b/Lib/test/test_clinic.py index 2f94f0168c9166..5e74b4fb77022d 100644 --- a/Lib/test/test_clinic.py +++ b/Lib/test/test_clinic.py @@ -4,14 +4,12 @@ from test import support, test_tools from test.support import os_helper -from test.support import SHORT_TIMEOUT, requires_subprocess from test.support.os_helper import TESTFN, unlink from textwrap import dedent from unittest import TestCase import collections import inspect import os.path -import subprocess import sys import unittest @@ -1411,30 +1409,26 @@ 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 run_clinic(self, *args): + with ( + support.captured_stdout() as out, + support.captured_stderr() as err, + self.assertRaises(SystemExit) as cm + ): + clinic.main(args) + return out.getvalue(), err.getvalue(), cm.exception.code def expect_success(self, *args): - return self._do_test(*args) + out, err, code = self.run_clinic(*args) + self.assertEqual(code, 0, f"Unexpected failure: {args=}") + self.assertEqual(err, "") + return out def expect_failure(self, *args): - return self._do_test(*args, expect_success=False) + out, err, code = self.run_clinic(*args) + self.assertNotEqual(code, 0, f"Unexpected success: {args=}") + return out, err def test_external(self): CLINIC_TEST = 'clinic.test.c' @@ -1498,8 +1492,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, "") @@ -1641,33 +1636,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") - msg = "Usage error: can't use -o with multiple filenames" - self.assertIn(msg, out) + _, err = self.expect_failure("-o", "out.c", "input.c", "moreinput.c") + msg = "error: can't use -o with multiple filenames" + 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", "") - msg = "Usage error: --srcdir must not be empty with --make" - self.assertIn(msg, out) + _, err = self.expect_failure("--make", "--srcdir", "") + msg = "error: --srcdir must not be empty with --make" + self.assertIn(msg, err) try: diff --git a/Misc/NEWS.d/next/Tools-Demos/2023-07-30-23-32-16.gh-issue-107467.5O9p3G.rst b/Misc/NEWS.d/next/Tools-Demos/2023-07-30-23-32-16.gh-issue-107467.5O9p3G.rst new file mode 100644 index 00000000000000..2996837371be0f --- /dev/null +++ b/Misc/NEWS.d/next/Tools-Demos/2023-07-30-23-32-16.gh-issue-107467.5O9p3G.rst @@ -0,0 +1,2 @@ +The Argument Clinic command-line tool now prints to stderr instead of stdout +on failure. diff --git a/Tools/clinic/clinic.py b/Tools/clinic/clinic.py index defe703b825b16..3501c16a567ccc 100755 --- a/Tools/clinic/clinic.py +++ b/Tools/clinic/clinic.py @@ -7,6 +7,7 @@ from __future__ import annotations import abc +import argparse import ast import builtins as bltns import collections @@ -5620,10 +5621,9 @@ def state_terminal(self, line: str | None) -> None: clinic = None -def main(argv: list[str]) -> None: - import sys - import argparse +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 +5646,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(parser: argparse.ArgumentParser, 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) + parser.error( + "can't specify --converters and a filename at the same time" + ) converters: list[tuple[str, str]] = [] return_converters: list[tuple[str, str]] = [] ignored = set(""" @@ -5707,19 +5708,13 @@ def main(argv: list[str]) -> None: print() print("All converters also accept (c_default=None, py_default=None, annotation=None).") print("All return converters also accept (py_default=None).") - sys.exit(0) + return 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) + parser.error("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) + parser.error("--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,14 +5730,10 @@ def main(argv: list[str]) -> None: return if not ns.filename: - cmdline.print_usage() - sys.exit(-1) + parser.error("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) + parser.error("can't use -o with multiple filenames") for filename in ns.filename: if ns.verbose: @@ -5750,6 +5741,12 @@ def main(argv: list[str]) -> None: parse_file(filename, output=ns.output, verify=not ns.force) -if __name__ == "__main__": - main(sys.argv[1:]) +def main(argv: list[str] | None = None) -> NoReturn: + parser = create_cli() + args = parser.parse_args(argv) + run_clinic(parser, args) sys.exit(0) + + +if __name__ == "__main__": + main()