Fix #121: Return CompletedProcess from shell cmds (#122)

Having a tuple is inconsistent and error-prone. This change in
`run_command` and `run_git_command` returns a CompletedProcess object
(same as with `subprocess.run`).

The CompletedProcess is an interoperable object from the standard
library and well-known. It contains everything we need so the caller
knows about the command, the return code, and outputs.

Author: tomschr

changelog.d/121.refactor.rst

@@ -0,0 +1,2 @@
+Refactor the ``run_command`` and ``run_git_command`` to return :class:`~subprocess.CompletedProcess`
+instead of tuples. This makes the two commands more consistent.

src/docbuild/cli/cmd_validate/process.py

@@ -91,7 +91,8 @@ async def validate_rng(
     more robust for complex XInclude statements, including those with XPointer.
 
     :param xmlfile: The path to the XML file to validate.
-    :param rng_schema_path: The path to the RELAX NG schema file. It supports both RNC and RNG formats.
+    :param rng_schema_path: The path to the RELAX NG schema file.
+        It supports both RNC and RNG formats.
     :param xinclude: If True, resolve XIncludes with `xmllint` before validation.
     :param idcheck: If True, perform ID uniqueness checks.
     :return: A tuple containing a boolean success status and any output message.
@@ -118,26 +119,27 @@ async def validate_rng(
                 tmp_filepath = Path(tmp_file.name)
 
                 # 1. Run xmllint to resolve XIncludes and save to temp file
-                returncode, _, stderr = await run_command(
-                    'xmllint', '--xinclude', '--output', str(tmp_filepath), str(xmlfile)
+                process = await run_command(
+                    ['xmllint', '--xinclude', '--output',
+                     str(tmp_filepath), str(xmlfile)]
                 )
-                if returncode != 0:
-                    return False, f'xmllint failed: {stderr.strip()}'
+                if process.returncode != 0:
+                    return False, f'xmllint failed: {process.stderr}'
 
                 # 2. Run jing on the resolved temporary file
                 jing_cmd.append(str(tmp_filepath))
-                returncode, stdout, stderr = await run_command(*jing_cmd)
-                if returncode != 0:
-                    return False, (stdout + stderr).strip()
+                process = await run_command(jing_cmd)
+                if process.returncode != 0:
+                    return False, (process.stdout + process.stderr).strip()
 
                 return True, ''
         else:
             # Validate directly with jing, no XInclude resolution.
             jing_cmd.append(str(xmlfile))
-            returncode, stdout, stderr = await run_command(*jing_cmd)
-            if returncode == 0:
+            process = await run_command(jing_cmd)
+            if process.returncode == 0:
                 return True, ''
-            return False, (stdout + stderr).strip()
+            return False, (process.stdout + process.stderr).strip()
 
     except FileNotFoundError as e:
         tool = e.filename or 'xmllint/jing'
@@ -151,8 +153,7 @@ def validate_rng_lxml(
     xmlfile: Path,
     rng_schema_path: Path = PRODUCT_CONFIG_SCHEMA,
 ) -> tuple[bool, str]:
-    """
-    Validate an XML file against a RELAX NG (.rng) schema using lxml.
+    """Validate an XML file against a RELAX NG (.rng) schema using lxml.
 
     This function uses lxml.etree.RelaxNG, which supports only the XML syntax
     of RELAX NG schemas (i.e., .rng files, not .rnc files).
@@ -324,7 +325,8 @@ async def process(
 
     # Filter for files that passed the initial validation
     successful_files_paths = [
-        xmlfile for xmlfile, result in zip(xmlfiles, results, strict=False) if result == 0
+        xmlfile for xmlfile, result in zip(xmlfiles, results, strict=False)
+        if result == 0
     ]
 
     # After validating individual files, perform a stitch validation to

src/docbuild/utils/git.py

@@ -4,7 +4,7 @@
 from pathlib import Path
 from typing import ClassVar, Self
 
-from ..constants import GIT_CONFIG_FILENAME, GITLOGGER_NAME
+from ..constants import GITLOGGER_NAME
 from ..models.repo import Repo
 from ..utils.shell import execute_git_command
 

src/docbuild/utils/shell.py

@@ -1,49 +1,57 @@
 """Shell command utilities."""
 
 import asyncio
+from collections.abc import Sequence
 import logging
 from pathlib import Path
+from subprocess import CompletedProcess
 
 from ..constants import GIT_CONFIG_FILENAME, GITLOGGER_NAME
 
 log = logging.getLogger(GITLOGGER_NAME)
 
 
 async def run_command(
-    *args: str,
+    args: Sequence[str],
+    *,
     cwd: Path | None = None,
     env: dict[str, str] | None = None,
-) -> tuple[int, str, str]:
+) -> CompletedProcess:
     """Run an external command and capture its output.
 
     :param args: The command and its arguments separated as tuple elements.
     :param cwd: The working directory for the command.
     :param env: A dictionary of environment variables for the new process.
-    :return: A tuple of (returncode, stdout, stderr).
+    :return: a :class:`~subprocess.CompletedProcess` object.
     :raises FileNotFoundError: if the command is not found.
     """
+    # 1. Start the subprocess and connect the pipes
+    # We always pipe because we must return the output strings.
     process = await asyncio.create_subprocess_exec(
-        *args,
+        args[0], *args[1:],
         cwd=cwd,
         stdout=asyncio.subprocess.PIPE,
         stderr=asyncio.subprocess.PIPE,
         env=env,
     )
-    stdout, stderr = await process.communicate()
-
-    # After .communicate() returns, the process has terminated and the
-    # returncode is guaranteed to be set to an integer.
-    assert process.returncode is not None
-
-    return process.returncode, stdout.decode(), stderr.decode()
+    # 2. Wait for the process to finish
+    bstdout, bstderr = await process.communicate()
+
+    # 3. Decode and return the result
+    return CompletedProcess(
+        args=args,
+        returncode=process.returncode or 0,
+        stdout=bstdout.decode('utf-8').strip(),
+        stderr=bstderr.decode('utf-8').strip(),
+    )
 
 
 async def execute_git_command(
     *args: str,
     cwd: Path | None = None,
     extra_env: dict[str, str] | None = None,
     gitconfig: Path | None = None,
-) -> tuple[str, str]:
+) -> CompletedProcess:
     """Execute a Git command asynchronously in a given directory.
 
     :param args: Command arguments for Git.
@@ -52,7 +60,7 @@ async def execute_git_command(
     :param extra_env: Additional environment variables to set for the command.
     :param gitconfig: The path to a separate Git configuration file. If None,
         the default config from etc/git/gitconfig is used.
-    :return: A tuple containing the decoded stdout and stderr of the command.
+    :return: a :class:`~subprocess.CompletedProcess` object.
     :raises RuntimeError: If the command fails.
     :raises FileNotFoundError: If `cwd` is specified but does not exist.
     """
@@ -82,16 +90,16 @@ async def execute_git_command(
     merged_env = {**default_env, **(extra_env or {})}
 
     # Delegate execution to the generic run_command utility
-    returncode, stdout, stderr = await run_command(
-        *command,
+    process = await run_command(
+        command,
         cwd=cwd,
         env=merged_env,
     )
 
-    if returncode != 0:
+    if process.returncode != 0:
         raise RuntimeError(
-            f'Git command failed with exit code {returncode}: '
-            f'{" ".join(command)}\nStderr: {stderr}'
+            f'Git command failed with exit code {process.returncode}: '
+            f'{" ".join(command)}\nStderr: {process.stderr}'
         )
 
-    return stdout.strip(), stderr.strip()
+    return process

tests/cli/cmd_repo/test_cmd_clone.py

@@ -1,5 +1,6 @@
 """Tests for the 'docbuild repo clone' command."""
 
+from subprocess import CompletedProcess
 import logging
 from unittest.mock import AsyncMock
 
@@ -67,9 +68,8 @@ def test_clone_from_xml_config(runner, tmp_path, mock_subprocess, caplog):
         },
     )
 
-    result = runner.invoke(clone, [], obj=context)
+    runner.invoke(clone, [], obj=context)
 
-    assert result.exit_code == 0, result.output
     assert mock_subprocess.call_count == 2
 
     calls = mock_subprocess.call_args_list

tests/cli/cmd_validate/test_process.py

@@ -1,5 +1,6 @@
 """Tests for the XML validation process module."""
 
+from subprocess import CompletedProcess
 from pathlib import Path
 from unittest.mock import AsyncMock, MagicMock, patch
 
@@ -33,7 +34,8 @@ def mock_context() -> DocBuildContext:
     return context
 
 
-@patch.object(process_module, 'validate_rng', new_callable=AsyncMock, return_value=(True, ''))
+@patch.object(process_module, 'validate_rng',
+              new_callable=AsyncMock, return_value=(True, ''))
 @patch.object(process_module.etree, 'parse', side_effect=ValueError('Generic test error'))
 async def test_process_file_with_generic_parsing_error(
     mock_etree_parse, mock_validate_rng, mock_context, tmp_path, capsys
@@ -119,7 +121,9 @@ async def test_process_file_with_unknown_validation_method(mock_context, tmp_pat
 @patch.object(process_module, 'run_command', new_callable=AsyncMock)
 async def test_validate_rng_with_idcheck_success(mock_run_command, tmp_path):
     """Test that validate_rng with idcheck=True succeeds for a valid XML."""
-    mock_run_command.return_value = (0, '', '')
+    mock_run_command.return_value = CompletedProcess(
+        args=["fake-command"], returncode=0, stdout='', stderr=''
+    )
     xml_file = tmp_path / 'valid.xml'
     xml_file.touch()
     rng_schema = tmp_path / 'schema.rnc'
@@ -130,13 +134,18 @@ async def test_validate_rng_with_idcheck_success(mock_run_command, tmp_path):
     assert is_valid is True
     assert message == ''
     # Ensure -i flag is passed to jing
-    assert '-i' in mock_run_command.call_args[0]
+    assert "-i" in mock_run_command.call_args.args[0]
 
 
 @patch.object(process_module, 'run_command', new_callable=AsyncMock)
 async def test_validate_rng_with_idcheck_duplicate_failure(mock_run_command, tmp_path):
     """Test that validate_rng with idcheck=True fails for a duplicate ID."""
-    mock_run_command.return_value = (1, '', 'error: duplicate ID "test-id"')
+    mock_run_command.return_value = CompletedProcess(
+        args=['fake-command'],
+        returncode=1,
+        stdout='',
+        stderr='error: duplicate ID "test-id"',
+    )
     xml_file = tmp_path / 'duplicate_id.xml'
     xml_file.touch()
     rng_schema = tmp_path / 'schema.rnc'
@@ -147,13 +156,18 @@ async def test_validate_rng_with_idcheck_duplicate_failure(mock_run_command, tmp
     assert is_valid is False
     assert 'duplicate ID' in message
     # Ensure -i flag is passed to jing
-    assert '-i' in mock_run_command.call_args[0]
+    assert '-i' in mock_run_command.call_args.args[0]
 
 
 @patch.object(process_module, 'run_command', new_callable=AsyncMock)
 async def test_validate_rng_without_idcheck_success(mock_run_command, tmp_path):
     """Test that validate_rng with idcheck=False succeeds despite a duplicate ID."""
-    mock_run_command.return_value = (0, '', '')
+    mock_run_command.return_value = CompletedProcess(
+        args=['fake-command'],
+        returncode=0,
+        stdout='',
+        stderr='',
+    )
     xml_file = tmp_path / 'duplicate_id.xml'
     xml_file.touch()
     rng_schema = tmp_path / 'schema.rnc'
@@ -164,4 +178,4 @@ async def test_validate_rng_without_idcheck_success(mock_run_command, tmp_path):
     assert is_valid is True
     assert message == ''
     # Ensure -i flag is NOT passed to jing
-    assert '-i' not in mock_run_command.call_args[0]
+    assert '-i' not in mock_run_command.call_args.args[0]

tests/cli/cmd_validate/validate/test_process_validation.py

@@ -1,3 +1,4 @@
+from subprocess import CompletedProcess
 import shutil
 import pytest
 from pathlib import Path
@@ -15,11 +16,13 @@ async def test_run_command():
     """Test the run_command function."""
     command = ['echo', 'Hello, World!']
 
-    returncode, stdout, stderr = await process_mod.run_command(*command)
+    process = await process_mod.run_command(command)
 
-    assert returncode == 0, f'Expected return code 0, got {returncode}'
-    assert stdout.strip() == 'Hello, World!', f'Unexpected stdout: {stdout}'
-    assert stderr == '', f'Unexpected stderr: {stderr}'
+    assert (process.returncode == 0,
+            f'Expected return code 0, got {process.returncode}')
+    assert (process.stdout.strip() == 'Hello, World!',
+            f'Unexpected stdout: {process.stdout}')
+    assert process.stderr == '', f'Unexpected stderr: {process.stderr}'
 
 
 @pytest.mark.skipif(not is_jing_installed(), reason="jing command not found")
@@ -99,20 +102,26 @@ async def test_validate_rng_jing_failure():
     rng_schema = MagicMock(spec=Path)
     xmlfile.__str__.return_value = '/mocked/path/to/file.xml'
     rng_schema.__str__.return_value = '/mocked/path/to/schema.rng'
- 
+
     with patch.object(
         process_mod,
         'run_command',
-        new=AsyncMock(return_value=(1, 'Error in jing', '')),
+        new=AsyncMock(return_value=CompletedProcess(
+                      args=['jing', xmlfile, rng_schema],
+                      returncode=1,
+                      stdout='Error in jing',
+                      stderr=''))
     ) as mock_run_command:
         success, output = await process_mod.validate_rng(
             xmlfile, rng_schema_path=rng_schema, xinclude=False, idcheck=False
         )
- 
+
         assert not success, 'Expected validation to fail.'
         assert output == 'Error in jing', f'Unexpected output: {output}'
- 
-        mock_run_command.assert_called_once_with('jing', str(rng_schema), str(xmlfile))
+
+        mock_run_command.assert_called_once_with(
+            ['jing', str(rng_schema), str(xmlfile)]
+        )
 
 
 async def test_validate_rng_command_not_found():
@@ -185,7 +194,7 @@ async def test_process_file_with_xmlsyntax_error(capsys, tmp_path):
 
     mock_context = Mock(spec=DocBuildContext)
     mock_context.verbose = 2
-    mock_context.validation_method = 'jing'  
+    mock_context.validation_method = 'jing'
 
     with (
         patch.object(