From 84b38f7b89fa22bea8bc70b03e664cd6cda9db84 Mon Sep 17 00:00:00 2001 From: marsha Date: Sun, 30 Oct 2022 14:47:42 -0500 Subject: [PATCH] Change `cmd_output_b`s `retcode` arg to a boolean `check` --- pre_commit/commands/run.py | 4 ++-- pre_commit/error_handler.py | 2 +- pre_commit/git.py | 4 ++-- pre_commit/languages/node.py | 2 +- pre_commit/languages/rust.py | 2 +- pre_commit/staged_files_only.py | 2 +- pre_commit/util.py | 11 ++++++----- pre_commit/xargs.py | 2 +- tests/commands/init_templatedir_test.py | 2 +- tests/commands/install_uninstall_test.py | 6 +++--- tests/commands/run_test.py | 4 ++-- tests/conftest.py | 2 +- tests/error_handler_test.py | 3 ++- tests/git_test.py | 2 +- tests/util_test.py | 6 +++--- 15 files changed, 28 insertions(+), 26 deletions(-) diff --git a/pre_commit/commands/run.py b/pre_commit/commands/run.py index 37f78f74..429e04c6 100644 --- a/pre_commit/commands/run.py +++ b/pre_commit/commands/run.py @@ -263,7 +263,7 @@ def _all_filenames(args: argparse.Namespace) -> Collection[str]: def _get_diff() -> bytes: _, out, _ = cmd_output_b( - 'git', 'diff', '--no-ext-diff', '--ignore-submodules', retcode=None, + 'git', 'diff', '--no-ext-diff', '--ignore-submodules', check=False, ) return out @@ -318,7 +318,7 @@ def _has_unmerged_paths() -> bool: def _has_unstaged_config(config_file: str) -> bool: retcode, _, _ = cmd_output_b( 'git', 'diff', '--no-ext-diff', '--exit-code', config_file, - retcode=None, + check=False, ) # be explicit, other git errors don't mean it has an unstaged config. return retcode == 1 diff --git a/pre_commit/error_handler.py b/pre_commit/error_handler.py index 992f5cdc..d740ee3e 100644 --- a/pre_commit/error_handler.py +++ b/pre_commit/error_handler.py @@ -25,7 +25,7 @@ def _log_and_exit( error_msg = f'{msg}: {type(exc).__name__}: '.encode() + force_bytes(exc) output.write_line_b(error_msg) - _, git_version_b, _ = cmd_output_b('git', '--version', retcode=None) + _, git_version_b, _ = cmd_output_b('git', '--version', check=False) git_version = git_version_b.decode(errors='backslashreplace').rstrip() storedir = Store().directory diff --git a/pre_commit/git.py b/pre_commit/git.py index 439da7ad..37ed3a71 100644 --- a/pre_commit/git.py +++ b/pre_commit/git.py @@ -187,11 +187,11 @@ def head_rev(remote: str) -> str: def has_diff(*args: str, repo: str = '.') -> bool: cmd = ('git', 'diff', '--quiet', '--no-ext-diff', *args) - return cmd_output_b(*cmd, cwd=repo, retcode=None)[0] == 1 + return cmd_output_b(*cmd, cwd=repo, check=False)[0] == 1 def has_core_hookpaths_set() -> bool: - _, out, _ = cmd_output_b('git', 'config', 'core.hooksPath', retcode=None) + _, out, _ = cmd_output_b('git', 'config', 'core.hooksPath', check=False) return bool(out.strip()) diff --git a/pre_commit/languages/node.py b/pre_commit/languages/node.py index 39f30006..37a5b63f 100644 --- a/pre_commit/languages/node.py +++ b/pre_commit/languages/node.py @@ -75,7 +75,7 @@ def in_env( def health_check(prefix: Prefix, language_version: str) -> str | None: with in_env(prefix, language_version): - retcode, _, _ = cmd_output_b('node', '--version', retcode=None) + retcode, _, _ = cmd_output_b('node', '--version', check=False) if retcode != 0: # pragma: win32 no cover return f'`node --version` returned {retcode}' else: diff --git a/pre_commit/languages/rust.py b/pre_commit/languages/rust.py index 0c347b49..ef603bc0 100644 --- a/pre_commit/languages/rust.py +++ b/pre_commit/languages/rust.py @@ -36,7 +36,7 @@ def get_default_version() -> str: # Just detecting the executable does not suffice, because if rustup is # installed but no toolchain is available, then `cargo` exists but # cannot be used without installing a toolchain first. - if cmd_output_b('cargo', '--version', retcode=None)[0] == 0: + if cmd_output_b('cargo', '--version', check=False)[0] == 0: return 'system' else: return C.DEFAULT diff --git a/pre_commit/staged_files_only.py b/pre_commit/staged_files_only.py index 83d8a03e..172fb20b 100644 --- a/pre_commit/staged_files_only.py +++ b/pre_commit/staged_files_only.py @@ -52,7 +52,7 @@ def _unstaged_changes_cleared(patch_dir: str) -> Generator[None, None, None]: retcode, diff_stdout_binary, _ = cmd_output_b( 'git', 'diff-index', '--ignore-submodules', '--binary', '--exit-code', '--no-color', '--no-ext-diff', tree, '--', - retcode=None, + check=False, ) if retcode and diff_stdout_binary.strip(): patch_filename = f'patch{int(time.time())}-{os.getpid()}' diff --git a/pre_commit/util.py b/pre_commit/util.py index 8c296f4d..a935c2d8 100644 --- a/pre_commit/util.py +++ b/pre_commit/util.py @@ -124,7 +124,7 @@ def _oserror_to_output(e: OSError) -> tuple[int, bytes, None]: def cmd_output_b( *cmd: str, - retcode: int | None = 0, + check: bool = True, **kwargs: Any, ) -> tuple[int, bytes, bytes | None]: _setdefault_kwargs(kwargs) @@ -142,8 +142,9 @@ def cmd_output_b( stdout_b, stderr_b = proc.communicate() returncode = proc.returncode - if retcode is not None and retcode != returncode: - raise CalledProcessError(returncode, cmd, retcode, stdout_b, stderr_b) + SUCCESS = 0 + if check and returncode != SUCCESS: + raise CalledProcessError(returncode, cmd, SUCCESS, stdout_b, stderr_b) return returncode, stdout_b, stderr_b @@ -196,10 +197,10 @@ if os.name != 'nt': # pragma: win32 no cover def cmd_output_p( *cmd: str, - retcode: int | None = 0, + check: bool = True, **kwargs: Any, ) -> tuple[int, bytes, bytes | None]: - assert retcode is None + assert check is False assert kwargs['stderr'] == subprocess.STDOUT, kwargs['stderr'] _setdefault_kwargs(kwargs) diff --git a/pre_commit/xargs.py b/pre_commit/xargs.py index f2b3421a..e3af90ef 100644 --- a/pre_commit/xargs.py +++ b/pre_commit/xargs.py @@ -154,7 +154,7 @@ def xargs( run_cmd: tuple[str, ...], ) -> tuple[int, bytes, bytes | None]: return cmd_fn( - *run_cmd, retcode=None, stderr=subprocess.STDOUT, **kwargs, + *run_cmd, check=False, stderr=subprocess.STDOUT, **kwargs, ) threads = min(len(partitions), target_concurrency) diff --git a/tests/commands/init_templatedir_test.py b/tests/commands/init_templatedir_test.py index 64bfc8b4..28f29b77 100644 --- a/tests/commands/init_templatedir_test.py +++ b/tests/commands/init_templatedir_test.py @@ -135,7 +135,7 @@ def test_init_templatedir_skip_on_missing_config( retcode, output = git_commit( fn=cmd_output_mocked_pre_commit_home, tempdir_factory=tempdir_factory, - retcode=None, + check=False, ) assert retcode == commit_retcode diff --git a/tests/commands/install_uninstall_test.py b/tests/commands/install_uninstall_test.py index ae668ac9..379c03a4 100644 --- a/tests/commands/install_uninstall_test.py +++ b/tests/commands/install_uninstall_test.py @@ -126,7 +126,7 @@ def _get_commit_output(tempdir_factory, touch_file='foo', **kwargs): cmd_output('git', 'add', touch_file) return git_commit( fn=cmd_output_mocked_pre_commit_home, - retcode=None, + check=False, tempdir_factory=tempdir_factory, **kwargs, ) @@ -286,7 +286,7 @@ def test_environment_not_sourced(tempdir_factory, store): 'GIT_AUTHOR_EMAIL': os.environ['GIT_AUTHOR_EMAIL'], 'GIT_COMMITTER_EMAIL': os.environ['GIT_COMMITTER_EMAIL'], }, - retcode=None, + check=False, ) assert ret == 1 assert out == ( @@ -551,7 +551,7 @@ def _get_push_output(tempdir_factory, remote='origin', opts=()): return cmd_output_mocked_pre_commit_home( 'git', 'push', remote, 'HEAD:new_branch', *opts, tempdir_factory=tempdir_factory, - retcode=None, + check=False, )[:2] diff --git a/tests/commands/run_test.py b/tests/commands/run_test.py index ef865330..03d741e0 100644 --- a/tests/commands/run_test.py +++ b/tests/commands/run_test.py @@ -718,7 +718,7 @@ def test_non_ascii_hook_id(repo_with_passing_hook, tempdir_factory): with cwd(repo_with_passing_hook): _, stdout, _ = cmd_output_mocked_pre_commit_home( sys.executable, '-m', 'pre_commit.main', 'run', '☃', - retcode=None, tempdir_factory=tempdir_factory, + check=False, tempdir_factory=tempdir_factory, ) assert 'UnicodeDecodeError' not in stdout # Doesn't actually happen, but a reasonable assertion @@ -737,7 +737,7 @@ def test_stdout_write_bug_py26(repo_with_failing_hook, store, tempdir_factory): _, out = git_commit( fn=cmd_output_mocked_pre_commit_home, tempdir_factory=tempdir_factory, - retcode=None, + check=False, ) assert 'UnicodeEncodeError' not in out # Doesn't actually happen, but a reasonable assertion diff --git a/tests/conftest.py b/tests/conftest.py index 40c0c050..30761715 100644 --- a/tests/conftest.py +++ b/tests/conftest.py @@ -68,7 +68,7 @@ def _make_conflict(): bar_only_file.write('bar') cmd_output('git', 'add', 'bar_only_file') git_commit(msg=_make_conflict.__name__) - cmd_output('git', 'merge', 'foo', retcode=None) + cmd_output('git', 'merge', 'foo', check=False) @pytest.fixture diff --git a/tests/error_handler_test.py b/tests/error_handler_test.py index 47e2afaa..068149e3 100644 --- a/tests/error_handler_test.py +++ b/tests/error_handler_test.py @@ -183,10 +183,11 @@ def test_error_handler_no_tty(tempdir_factory): 'from pre_commit.error_handler import error_handler\n' 'with error_handler():\n' ' raise ValueError("\\u2603")\n', - retcode=3, + check=False, tempdir_factory=tempdir_factory, pre_commit_home=pre_commit_home, ) + assert ret == 3 log_file = os.path.join(pre_commit_home, 'pre-commit.log') out_lines = out.splitlines() assert out_lines[-2] == 'An unexpected error has occurred: ValueError: ☃' diff --git a/tests/git_test.py b/tests/git_test.py index b9f524a1..93f5a1c6 100644 --- a/tests/git_test.py +++ b/tests/git_test.py @@ -104,7 +104,7 @@ def test_is_in_merge_conflict_submodule(in_conflicting_submodule): def test_cherry_pick_conflict(in_merge_conflict): cmd_output('git', 'merge', '--abort') foo_ref = cmd_output('git', 'rev-parse', 'foo')[1].strip() - cmd_output('git', 'cherry-pick', foo_ref, retcode=None) + cmd_output('git', 'cherry-pick', foo_ref, check=False) assert git.is_in_merge_conflict() is False diff --git a/tests/util_test.py b/tests/util_test.py index 6b00f9fc..bc8f585f 100644 --- a/tests/util_test.py +++ b/tests/util_test.py @@ -83,14 +83,14 @@ def test_tmpdir(): def test_cmd_output_exe_not_found(): - ret, out, _ = cmd_output('dne', retcode=None) + ret, out, _ = cmd_output('dne', check=False) assert ret == 1 assert out == 'Executable `dne` not found' @pytest.mark.parametrize('fn', (cmd_output_b, cmd_output_p)) def test_cmd_output_exe_not_found_bytes(fn): - ret, out, _ = fn('dne', retcode=None, stderr=subprocess.STDOUT) + ret, out, _ = fn('dne', check=False, stderr=subprocess.STDOUT) assert ret == 1 assert out == b'Executable `dne` not found' @@ -101,7 +101,7 @@ def test_cmd_output_no_shebang(tmpdir, fn): make_executable(f) # previously this raised `OSError` -- the output is platform specific - ret, out, _ = fn(str(f), retcode=None, stderr=subprocess.STDOUT) + ret, out, _ = fn(str(f), check=False, stderr=subprocess.STDOUT) assert ret == 1 assert isinstance(out, bytes) assert out.endswith(b'\n')