From 6095a7baaa10dade3be63867114a6a0d0408ac6b Mon Sep 17 00:00:00 2001 From: shenxianpeng Date: Tue, 4 Nov 2025 19:20:29 +0200 Subject: [PATCH 1/7] fix: print stdout and stderr when there is error Co-authored-by: Brendan <2bndy5@gmail.com> --- cpp_linter_hooks/util.py | 9 ++++++--- 1 file changed, 6 insertions(+), 3 deletions(-) diff --git a/cpp_linter_hooks/util.py b/cpp_linter_hooks/util.py index ebd48a3..ba1fc28 100644 --- a/cpp_linter_hooks/util.py +++ b/cpp_linter_hooks/util.py @@ -64,11 +64,14 @@ def _install_tool(tool: str, version: str) -> Optional[Path]: try: subprocess.check_call( [sys.executable, "-m", "pip", "install", f"{tool}=={version}"], - stdout=subprocess.DEVNULL, - stderr=subprocess.DEVNULL, + capture_output=True, + check=True, ) return shutil.which(tool) - except subprocess.CalledProcessError: + except subprocess.CalledProcessError as e: + LOG.error("pip failed to install %s %s", tool, version) + LOG.error(e.stdout.decode(encoding="utf-8")) + LOG.error(e.stderr.decode(encoding="utf-8")) return None From 25b61071f926e959cecb75f31fd9cd6970accf82 Mon Sep 17 00:00:00 2001 From: Brendan <2bndy5@gmail.com> Date: Tue, 4 Nov 2025 11:15:16 -0800 Subject: [PATCH 2/7] switch to `subprocess.run()` --- cpp_linter_hooks/util.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/cpp_linter_hooks/util.py b/cpp_linter_hooks/util.py index ba1fc28..910a1d2 100644 --- a/cpp_linter_hooks/util.py +++ b/cpp_linter_hooks/util.py @@ -62,7 +62,7 @@ def parse_version(v: str): def _install_tool(tool: str, version: str) -> Optional[Path]: """Install a tool using pip, suppressing output.""" try: - subprocess.check_call( + subprocess.run( [sys.executable, "-m", "pip", "install", f"{tool}=={version}"], capture_output=True, check=True, From 81d8cab9d5ac7269e48d55ea65301eb98df82cac Mon Sep 17 00:00:00 2001 From: shenxianpeng Date: Tue, 4 Nov 2025 21:24:51 +0200 Subject: [PATCH 3/7] fix: update test adjust tests for `subprocess.run()` --- tests/test_util.py | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/tests/test_util.py b/tests/test_util.py index 8305ba0..2a90860 100644 --- a/tests/test_util.py +++ b/tests/test_util.py @@ -124,16 +124,16 @@ def test_install_tool_success(): mock_path = "/usr/bin/clang-format" with ( - patch("subprocess.check_call") as mock_check_call, + patch("subprocess.run") as mock_run, patch("shutil.which", return_value=mock_path), ): result = _install_tool("clang-format", "20.1.7") assert result == mock_path - mock_check_call.assert_called_once_with( + mock_run.assert_called_once_with( [sys.executable, "-m", "pip", "install", "clang-format==20.1.7"], - stdout=subprocess.DEVNULL, - stderr=subprocess.DEVNULL, + capture_output=True, + check=True, ) @@ -142,7 +142,7 @@ def test_install_tool_failure(): """Test _install_tool when pip install fails.""" with ( patch( - "subprocess.check_call", + "subprocess.run", side_effect=subprocess.CalledProcessError(1, ["pip"]), ), patch("cpp_linter_hooks.util.LOG"), @@ -154,7 +154,7 @@ def test_install_tool_failure(): @pytest.mark.benchmark def test_install_tool_success_but_not_found(): """Test _install_tool when install succeeds but tool not found in PATH.""" - with patch("subprocess.check_call"), patch("shutil.which", return_value=None): + with patch("subprocess.run"), patch("shutil.which", return_value=None): result = _install_tool("clang-format", "20.1.7") assert result is None From fa6bdbbc51883e241cff830faa4ce6a404a078c9 Mon Sep 17 00:00:00 2001 From: Brendan <2bndy5@gmail.com> Date: Tue, 4 Nov 2025 11:25:50 -0800 Subject: [PATCH 4/7] style(typing): conditionally import toml lib per Python version --- cpp_linter_hooks/util.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/cpp_linter_hooks/util.py b/cpp_linter_hooks/util.py index 910a1d2..cca2158 100644 --- a/cpp_linter_hooks/util.py +++ b/cpp_linter_hooks/util.py @@ -5,9 +5,9 @@ import logging from typing import Optional, List -try: +if sys.version_info >= (3, 11): import tomllib -except ModuleNotFoundError: +else: import tomli as tomllib from cpp_linter_hooks.versions import CLANG_FORMAT_VERSIONS, CLANG_TIDY_VERSIONS From 15348719473aa7433d3420a055545e947405a8ee Mon Sep 17 00:00:00 2001 From: Brendan <2bndy5@gmail.com> Date: Tue, 4 Nov 2025 12:41:04 -0800 Subject: [PATCH 5/7] fix: capture output and patch test's mocks --- cpp_linter_hooks/util.py | 17 ++++++++--------- tests/test_util.py | 17 +++++++++++------ 2 files changed, 19 insertions(+), 15 deletions(-) diff --git a/cpp_linter_hooks/util.py b/cpp_linter_hooks/util.py index cca2158..680f309 100644 --- a/cpp_linter_hooks/util.py +++ b/cpp_linter_hooks/util.py @@ -61,17 +61,16 @@ def parse_version(v: str): def _install_tool(tool: str, version: str) -> Optional[Path]: """Install a tool using pip, suppressing output.""" - try: - subprocess.run( - [sys.executable, "-m", "pip", "install", f"{tool}=={version}"], - capture_output=True, - check=True, - ) + result = subprocess.run( + [sys.executable, "-m", "pip", "install", f"{tool}=={version}"], + capture_output=True, + ) + if result.returncode == 0: return shutil.which(tool) - except subprocess.CalledProcessError as e: + else: LOG.error("pip failed to install %s %s", tool, version) - LOG.error(e.stdout.decode(encoding="utf-8")) - LOG.error(e.stderr.decode(encoding="utf-8")) + LOG.error(result.stdout.decode(encoding="utf-8")) + LOG.error(result.stderr.decode(encoding="utf-8")) return None diff --git a/tests/test_util.py b/tests/test_util.py index 2a90860..7f51b4e 100644 --- a/tests/test_util.py +++ b/tests/test_util.py @@ -123,8 +123,11 @@ def test_install_tool_success(): """Test _install_tool successful installation.""" mock_path = "/usr/bin/clang-format" + def patched_run(*args, **kwargs): + return subprocess.CompletedProcess(args, returncode=0) + with ( - patch("subprocess.run") as mock_run, + patch("subprocess.run", side_effect=patched_run) as mock_run, patch("shutil.which", return_value=mock_path), ): result = _install_tool("clang-format", "20.1.7") @@ -133,18 +136,20 @@ def test_install_tool_success(): mock_run.assert_called_once_with( [sys.executable, "-m", "pip", "install", "clang-format==20.1.7"], capture_output=True, - check=True, ) @pytest.mark.benchmark def test_install_tool_failure(): """Test _install_tool when pip install fails.""" + + def patched_run(*args, **kwargs): + return subprocess.CompletedProcess( + args, returncode=1, stderr=b"Error", stdout=b"Installation failed" + ) + with ( - patch( - "subprocess.run", - side_effect=subprocess.CalledProcessError(1, ["pip"]), - ), + patch("subprocess.run", side_effect=patched_run), patch("cpp_linter_hooks.util.LOG"), ): result = _install_tool("clang-format", "20.1.7") From 2a96fdf20d2d74b3728a070618458d408b2cec54 Mon Sep 17 00:00:00 2001 From: Brendan <2bndy5@gmail.com> Date: Tue, 4 Nov 2025 13:15:55 -0800 Subject: [PATCH 6/7] AI review changes --- cpp_linter_hooks/util.py | 7 ++++--- tests/test_util.py | 12 ++++++++++-- 2 files changed, 14 insertions(+), 5 deletions(-) diff --git a/cpp_linter_hooks/util.py b/cpp_linter_hooks/util.py index 680f309..bed169b 100644 --- a/cpp_linter_hooks/util.py +++ b/cpp_linter_hooks/util.py @@ -60,17 +60,18 @@ def parse_version(v: str): def _install_tool(tool: str, version: str) -> Optional[Path]: - """Install a tool using pip, suppressing output.""" + """Install a tool using pip, logging output on failure.""" result = subprocess.run( [sys.executable, "-m", "pip", "install", f"{tool}=={version}"], capture_output=True, + text=True, ) if result.returncode == 0: return shutil.which(tool) else: LOG.error("pip failed to install %s %s", tool, version) - LOG.error(result.stdout.decode(encoding="utf-8")) - LOG.error(result.stderr.decode(encoding="utf-8")) + LOG.error(result.stdout) + LOG.error(result.stderr) return None diff --git a/tests/test_util.py b/tests/test_util.py index 7f51b4e..3414f53 100644 --- a/tests/test_util.py +++ b/tests/test_util.py @@ -136,6 +136,7 @@ def patched_run(*args, **kwargs): mock_run.assert_called_once_with( [sys.executable, "-m", "pip", "install", "clang-format==20.1.7"], capture_output=True, + text=True, ) @@ -145,7 +146,7 @@ def test_install_tool_failure(): def patched_run(*args, **kwargs): return subprocess.CompletedProcess( - args, returncode=1, stderr=b"Error", stdout=b"Installation failed" + args, returncode=1, stderr="Error", stdout="Installation failed" ) with ( @@ -159,7 +160,14 @@ def patched_run(*args, **kwargs): @pytest.mark.benchmark def test_install_tool_success_but_not_found(): """Test _install_tool when install succeeds but tool not found in PATH.""" - with patch("subprocess.run"), patch("shutil.which", return_value=None): + + def patched_run(*args, **kwargs): + return subprocess.CompletedProcess(args, returncode=0) + + with ( + patch("subprocess.run", side_effect=patched_run), + patch("shutil.which", return_value=None), + ): result = _install_tool("clang-format", "20.1.7") assert result is None From 3f12fff31743fa1c84f5f6f3c7e46b2308f9015e Mon Sep 17 00:00:00 2001 From: Brendan <2bndy5@gmail.com> Date: Tue, 4 Nov 2025 17:17:32 -0800 Subject: [PATCH 7/7] remove redundant `else` --- cpp_linter_hooks/util.py | 9 ++++----- 1 file changed, 4 insertions(+), 5 deletions(-) diff --git a/cpp_linter_hooks/util.py b/cpp_linter_hooks/util.py index bed169b..cfd4209 100644 --- a/cpp_linter_hooks/util.py +++ b/cpp_linter_hooks/util.py @@ -68,11 +68,10 @@ def _install_tool(tool: str, version: str) -> Optional[Path]: ) if result.returncode == 0: return shutil.which(tool) - else: - LOG.error("pip failed to install %s %s", tool, version) - LOG.error(result.stdout) - LOG.error(result.stderr) - return None + LOG.error("pip failed to install %s %s", tool, version) + LOG.error(result.stdout) + LOG.error(result.stderr) + return None def resolve_install(tool: str, version: Optional[str]) -> Optional[Path]: