From ba7bc573ad7357866b69d67366b095f3bb4bfb63 Mon Sep 17 00:00:00 2001 From: Mike Frysinger Date: Wed, 11 Nov 2026 21:23:33 -0500 Subject: [PATCH] typing: fix various errors caught by pyrefly These should be fairly straightforward and not require much thought. I've punted the more complicated typing changes. Change-Id: I79167f07e0ec259586520104f07e4cee096a382e Reviewed-on: https://gerrit-review.googlesource.com/c/git-repo/+/656663 Tested-by: Mike Frysinger Commit-Queue: Mike Frysinger Reviewed-by: Gavin Mak --- color.py | 2 +- command.py | 11 +++++++---- error.py | 6 +++--- git_command.py | 4 ++-- git_superproject.py | 6 +++--- hooks.py | 3 +++ platform_utils.py | 3 ++- repo | 14 ++++++-------- repo_trace.py | 6 +++++- subcmds/checkout.py | 6 +++--- subcmds/forall.py | 2 +- subcmds/grep.py | 3 ++- subcmds/info.py | 2 +- subcmds/init.py | 2 +- subcmds/start.py | 4 ++-- tests/e2e/test_harness.py | 2 ++ tests/test_editor.py | 4 +++- tests/test_git_command.py | 4 ++-- tests/test_git_config.py | 3 +++ tests/test_git_refs.py | 3 +++ tests/test_manifest_xml.py | 4 +++- tests/test_repo_trace.py | 1 + tests/test_ssh.py | 8 ++++++-- tests/test_subcmds.py | 1 + tests/test_subcmds_upload.py | 4 ++-- tests/utils_for_test.py | 2 +- wrapper.py | 1 + 27 files changed, 70 insertions(+), 41 deletions(-) diff --git a/color.py b/color.py index 53fadf0a1..a5582e077 100644 --- a/color.py +++ b/color.py @@ -82,7 +82,7 @@ def _Color(fg=None, bg=None, attr=None): return code -DEFAULT = None +DEFAULT: Optional[str] = None class _CheckConsoleSentinel: diff --git a/command.py b/command.py index 26e45de7d..5c725a0f7 100644 --- a/command.py +++ b/command.py @@ -43,7 +43,7 @@ WORKER_BATCH_SIZE = 32 # How many jobs to run in parallel by default? This assumes the jobs are # largely I/O bound and do not hit the network. -DEFAULT_LOCAL_JOBS = min(os.cpu_count(), 8) +DEFAULT_LOCAL_JOBS = min(os.cpu_count() or 1, 8) class UsageError(error.RepoExitError): @@ -105,7 +105,8 @@ class Command: # Cache for the OptionParser property. self._optparse = None - def WantPager(self, _opt): + def WantPager(self, opt) -> bool: + del opt # Unused. return False @staticmethod @@ -623,7 +624,8 @@ class InteractiveCommand(Command): within a pager, even if the user asks to. """ - def WantPager(self, _opt): + def WantPager(self, opt) -> bool: + del opt # Unused. return False @@ -632,7 +634,8 @@ class PagedCommand(Command): larger than one screen full. """ - def WantPager(self, _opt): + def WantPager(self, opt) -> bool: + del opt # Unused. return True diff --git a/error.py b/error.py index 1a65f365c..c608b60d3 100644 --- a/error.py +++ b/error.py @@ -12,7 +12,7 @@ # See the License for the specific language governing permissions and # limitations under the License. -from typing import List +from typing import Optional, Sequence class BaseRepoError(Exception): @@ -22,7 +22,7 @@ class BaseRepoError(Exception): class RepoError(BaseRepoError): """Exceptions thrown inside repo that can be handled.""" - def __init__(self, *args, project: str = None) -> None: + def __init__(self, *args, project: Optional[str] = None) -> None: super().__init__(*args) self.project = project @@ -36,7 +36,7 @@ class RepoExitError(BaseRepoError): self, *args, exit_code: int = 1, - aggregate_errors: List[Exception] = None, + aggregate_errors: Optional[Sequence[Exception]] = None, **kwargs, ) -> None: super().__init__(*args, **kwargs) diff --git a/git_command.py b/git_command.py index 63fd3c7a6..1cc670f06 100644 --- a/git_command.py +++ b/git_command.py @@ -18,7 +18,7 @@ import os import re import subprocess import sys -from typing import Any, Optional +from typing import Any, Dict, Optional import error import git_trace2_event_log_base @@ -220,7 +220,7 @@ def git_require(min_version, fail=False, msg=""): return False -def _get_basic_env() -> None: +def _get_basic_env() -> Dict[str, str]: """Return a basic env for running git under. This is guaranteed to be side-effect free. diff --git a/git_superproject.py b/git_superproject.py index cbe9cb6ec..988c6eae2 100644 --- a/git_superproject.py +++ b/git_superproject.py @@ -29,7 +29,7 @@ import os import sys import tempfile import time -from typing import NamedTuple +from typing import NamedTuple, Optional import urllib.parse import git_command @@ -54,7 +54,7 @@ class CommitIdsResult(NamedTuple): """Return the commit ids and whether caller should exit.""" # A dictionary with the projects/commit ids on success, otherwise None. - commit_ids: dict + commit_ids: Optional[dict] # Whether the caller should exit. fatal: bool @@ -63,7 +63,7 @@ class UpdateProjectsResult(NamedTuple): """Return the overriding manifest file and whether caller should exit.""" # Path name of the overriding manifest file if successful, otherwise None. - manifest_path: str + manifest_path: Optional[str] # Whether the caller should exit. fatal: bool diff --git a/hooks.py b/hooks.py index d9c44ecaf..d86691d7a 100644 --- a/hooks.py +++ b/hooks.py @@ -323,6 +323,7 @@ class RepoHook: """ # Exec, storing global context in the context dict. We catch exceptions # and convert to a HookError w/ just the failing traceback. + assert self._script_fullpath is not None try: exec(compile(data, self._script_fullpath, "exec"), context) except Exception: @@ -367,6 +368,7 @@ class RepoHook: # Always run hooks with CWD as topdir. os.chdir(self._repo_topdir) + assert self._script_fullpath is not None # Put the hook dir as the first item of sys.path so hooks can do # relative imports. We want to replace the repo dir as [0] so # hooks can't import repo files. @@ -407,6 +409,7 @@ class RepoHook: def _CheckHook(self): # Bail with a nice error if we can't find the hook. + assert self._script_fullpath is not None if not os.path.isfile(self._script_fullpath): raise error.HookError( "Couldn't find repo hook: %s" % self._script_fullpath diff --git a/platform_utils.py b/platform_utils.py index f2fe66749..e2e4af952 100644 --- a/platform_utils.py +++ b/platform_utils.py @@ -17,6 +17,7 @@ import os import platform import shutil import stat +from typing import Union def isWindows(): @@ -224,7 +225,7 @@ def rmdir(path): os.rmdir(_makelongpath(path)) -def removedirs(path: str) -> None: +def removedirs(path: Union[str, os.PathLike]) -> None: """Remove a directory tree of symlinks and empty directories. Walks |path| bottom-up without following symlinks. Removes symlinks diff --git a/repo b/repo index ce34ca6ec..cbe9519a1 100755 --- a/repo +++ b/repo @@ -26,7 +26,7 @@ import platform import shlex import subprocess import sys -from typing import NamedTuple, Tuple +from typing import List, NamedTuple, Tuple # These should never be newer than the main.py version since this needs to be a @@ -119,12 +119,10 @@ if __name__ == "__main__": # repo default configuration # -REPO_URL = os.environ.get("REPO_URL", None) -if not REPO_URL: - REPO_URL = "https://gerrit.googlesource.com/git-repo" -REPO_REV = os.environ.get("REPO_REV") -if not REPO_REV: - REPO_REV = "stable" +REPO_URL = ( + os.environ.get("REPO_URL") or "https://gerrit.googlesource.com/git-repo" +) +REPO_REV = os.environ.get("REPO_REV") or "stable" # URL to file bug reports for repo tool issues. BUG_URL = "https://issues.gerritcodereview.com/issues/new?component=1370071" @@ -902,7 +900,7 @@ def _GetRepoConfig(name): def _InitHttp(): - handlers = [] + handlers: List[urllib.request.BaseHandler] = [] mgr = urllib.request.HTTPPasswordMgrWithDefaultRealm() try: diff --git a/repo_trace.py b/repo_trace.py index d8ed58329..89d5e3f12 100644 --- a/repo_trace.py +++ b/repo_trace.py @@ -25,6 +25,7 @@ import os import sys import tempfile import time +from typing import Optional import platform_utils @@ -35,7 +36,7 @@ REPO_TRACE = "REPO_TRACE" # Temporarily set tracing to always on unless user expicitly sets to 0. _TRACE = os.environ.get(REPO_TRACE) != "0" _TRACE_TO_STDERR = False -_TRACE_FILE = None +_TRACE_FILE: Optional[str] = None _TRACE_FILE_NAME = "TRACE_FILE" _MAX_SIZE = 70 # in MiB _NEW_COMMAND_SEP = "+++++++++++++++NEW COMMAND+++++++++++++++++++" @@ -104,6 +105,7 @@ class Trace(contextlib.ContextDecorator): f"PID: {os.getpid()} START: {self._time()} :{self._trace_msg}\n" ) + assert _TRACE_FILE is not None with open(_TRACE_FILE, "a") as f: print(print_msg, file=f) @@ -120,6 +122,7 @@ class Trace(contextlib.ContextDecorator): f"PID: {os.getpid()} END: {self._time()} :{self._trace_msg}\n" ) + assert _TRACE_FILE is not None with open(_TRACE_FILE, "a") as f: print(print_msg, file=f) @@ -141,6 +144,7 @@ def _GetTraceFile(quiet): def _ClearOldTraces(): """Clear the oldest commands if trace file is too big.""" + assert _TRACE_FILE is not None try: with open(_TRACE_FILE, errors="ignore") as f: if os.path.getsize(f.name) / (1024 * 1024) <= _MAX_SIZE: diff --git a/subcmds/checkout.py b/subcmds/checkout.py index 218fcb11f..71a624a09 100644 --- a/subcmds/checkout.py +++ b/subcmds/checkout.py @@ -13,7 +13,7 @@ # limitations under the License. import functools -from typing import NamedTuple +from typing import NamedTuple, Optional import command import error @@ -26,9 +26,9 @@ logger = repo_logging.RepoLogger(__file__) class CheckoutBranchResult(NamedTuple): # Whether the Project is on the branch (i.e. branch exists and no errors) - result: bool + result: Optional[bool] project_idx: int - error: Exception + error: Optional[Exception] class CheckoutCommandError(error.RepoExitError): diff --git a/subcmds/forall.py b/subcmds/forall.py index 135150c02..617815945 100644 --- a/subcmds/forall.py +++ b/subcmds/forall.py @@ -195,7 +195,7 @@ without iterating through the remaining projects. "--interactive", action="store_true", help="force interactive usage" ) - def WantPager(self, opt): + def WantPager(self, opt) -> bool: return opt.project_header and opt.jobs == 1 def ValidateOptions(self, opt, args): diff --git a/subcmds/grep.py b/subcmds/grep.py index df6be2769..baa096035 100644 --- a/subcmds/grep.py +++ b/subcmds/grep.py @@ -102,8 +102,9 @@ contain a line that matches both expressions: if value is not None: pt.append(value) - def _CommonOptions(self, p): + def _CommonOptions(self, p, opt_v=True): """Override common options slightly.""" + del opt_v # Unused. super()._CommonOptions(p, opt_v=False) def _Options(self, p): diff --git a/subcmds/info.py b/subcmds/info.py index bda4b3e99..1382524f7 100644 --- a/subcmds/info.py +++ b/subcmds/info.py @@ -130,7 +130,7 @@ class Info(command.PagedCommand): help=f"output format: {', '.join(formats)} (default: %default)", ) - def WantPager(self, opt): + def WantPager(self, opt) -> bool: return OutputFormat[opt.format.upper()] == OutputFormat.TEXT def ValidateOptions(self, opt, args): diff --git a/subcmds/init.py b/subcmds/init.py index b42355b56..0eb2444e8 100644 --- a/subcmds/init.py +++ b/subcmds/init.py @@ -89,7 +89,7 @@ manifest, a subsequent `repo sync` (or `repo sync -d`) is necessary to update the working directory files. """ - def _CommonOptions(self, p): + def _CommonOptions(self, p, opt_v=True): """Disable due to re-use of Wrapper().""" def _Options(self, p): diff --git a/subcmds/start.py b/subcmds/start.py index 9b9800b6c..6fb155026 100644 --- a/subcmds/start.py +++ b/subcmds/start.py @@ -13,7 +13,7 @@ # limitations under the License. import functools -from typing import NamedTuple +from typing import NamedTuple, Optional import command import error @@ -28,7 +28,7 @@ logger = repo_logging.RepoLogger(__file__) class ExecuteOneResult(NamedTuple): project_idx: int - error: Exception + error: Optional[Exception] class StartError(error.RepoExitError): diff --git a/tests/e2e/test_harness.py b/tests/e2e/test_harness.py index 63baf602b..41beaf55b 100644 --- a/tests/e2e/test_harness.py +++ b/tests/e2e/test_harness.py @@ -28,6 +28,7 @@ def _http_request( ) -> Tuple[int, bytes]: """Send a request to `server` & return the response status & body.""" url = urllib.parse.urlsplit(server.url) + assert url.hostname is not None conn = http.client.HTTPConnection(url.hostname, url.port, timeout=30) try: conn.request(method, path) @@ -113,6 +114,7 @@ def test_http_post_body( if expect_continue: headers["Expect"] = "100-continue" url = urllib.parse.urlsplit(git_server.url) + assert url.hostname is not None conn = http.client.HTTPConnection(url.hostname, url.port, timeout=30) try: # http.client skips any interim `100 Continue` response. diff --git a/tests/test_editor.py b/tests/test_editor.py index f7a37fef8..35dfc0fa6 100644 --- a/tests/test_editor.py +++ b/tests/test_editor.py @@ -14,13 +14,15 @@ """Unittests for the editor.py module.""" +from typing import Iterator + import pytest import editor @pytest.fixture(autouse=True) -def reset_editor() -> None: +def reset_editor() -> Iterator[None]: """Take care of resetting Editor state across tests.""" editor.Editor._editor = None yield diff --git a/tests/test_git_command.py b/tests/test_git_command.py index 903322f07..7d4776b8d 100644 --- a/tests/test_git_command.py +++ b/tests/test_git_command.py @@ -444,13 +444,13 @@ class GitRequireTests(unittest.TestCase): """Test fatal require calls with old versions.""" with self.assertRaises(git_command.GitRequireError) as e: git_command.git_require((2,), fail=True) - self.assertNotEqual(0, e.code) + self.assertNotEqual(0, e.exception.exit_code) def test_older_fatal_msg(self): """Test fatal require calls with old versions and message.""" with self.assertRaises(git_command.GitRequireError) as e: git_command.git_require((2,), fail=True, msg="so sad") - self.assertNotEqual(0, e.code) + self.assertNotEqual(0, e.exception.exit_code) class GitCommandErrorTest(unittest.TestCase): diff --git a/tests/test_git_config.py b/tests/test_git_config.py index f4a901cf8..8587a6533 100644 --- a/tests/test_git_config.py +++ b/tests/test_git_config.py @@ -214,6 +214,9 @@ def test_get_sync_analysis_state_data(rw_config_file: Path) -> None: class Options: """Container for testing.""" + verbose: str + mp_update: str + options = Options() options.verbose = "true" options.mp_update = "false" diff --git a/tests/test_git_refs.py b/tests/test_git_refs.py index 345b17025..5abf2e7a2 100644 --- a/tests/test_git_refs.py +++ b/tests/test_git_refs.py @@ -141,6 +141,7 @@ def test_root_ref_snapshot_falls_back_for_unborn_head( def read_head(name: str) -> None: assert name == "HEAD" + assert refs._symref is not None refs._symref[name] = "refs/heads/main" monkeypatch.setattr(refs, "_ReadSymbolicRef", read_head) @@ -171,6 +172,7 @@ def test_old_git_keeps_separate_head_fallback( def read_head(name: str) -> None: assert name == "HEAD" + assert refs._symref is not None refs._symref[name] = "refs/heads/main" monkeypatch.setattr(refs, "_ReadSymbolicRef", read_head) @@ -204,6 +206,7 @@ def test_for_each_ref_failure_falls_back_to_symbolic_ref( def read_head(name: str) -> None: assert name == "HEAD" + assert refs._symref is not None refs._symref[name] = "refs/heads/main" monkeypatch.setattr(refs, "_ReadSymbolicRef", read_head) diff --git a/tests/test_manifest_xml.py b/tests/test_manifest_xml.py index 2894e2df5..9011f07f8 100644 --- a/tests/test_manifest_xml.py +++ b/tests/test_manifest_xml.py @@ -180,7 +180,9 @@ class TestValue: """Check utility parsing code.""" def _get_node(self, text: str) -> xml.dom.minidom.Element: - return xml.dom.minidom.parseString(text).firstChild + node = xml.dom.minidom.parseString(text).firstChild + assert isinstance(node, xml.dom.minidom.Element) + return node def test_bool_default(self) -> None: """Check XmlBool default handling.""" diff --git a/tests/test_repo_trace.py b/tests/test_repo_trace.py index 7b938a6f6..1187320e1 100644 --- a/tests/test_repo_trace.py +++ b/tests/test_repo_trace.py @@ -27,6 +27,7 @@ def test_trace_max_size_enforced(monkeypatch: pytest.MonkeyPatch) -> None: # Enable trace for the test, in case users have it disabled. monkeypatch.setattr(repo_trace, "_TRACE", True) + assert repo_trace._TRACE_FILE is not None with repo_trace.Trace(content, first_trace=True): pass diff --git a/tests/test_ssh.py b/tests/test_ssh.py index 26bd98518..a0336c855 100644 --- a/tests/test_ssh.py +++ b/tests/test_ssh.py @@ -91,11 +91,15 @@ def test_ssh_sock(monkeypatch: pytest.MonkeyPatch) -> None: # Old ssh version uses port. with mock.patch.object(ssh, "version", return_value=(6, 6)): with proxy as ssh_proxy: - assert ssh_proxy.sock().endswith("%p") + sock = ssh_proxy.sock() + assert sock is not None + assert sock.endswith("%p") proxy._sock_path = None # New ssh version uses hash. with mock.patch.object(ssh, "version", return_value=(6, 7)): with proxy as ssh_proxy: - assert ssh_proxy.sock().endswith("%C") + sock = ssh_proxy.sock() + assert sock is not None + assert sock.endswith("%C") proxy._sock_path = None diff --git a/tests/test_subcmds.py b/tests/test_subcmds.py index 17661b97c..587f298f0 100644 --- a/tests/test_subcmds.py +++ b/tests/test_subcmds.py @@ -112,6 +112,7 @@ def test_cli_option_dest() -> None: bad_opts.append((str(opt), opt.dest)) # Hook the option check list. + assert optparse.Option.CHECK_METHODS is not None optparse.Option.CHECK_METHODS.insert(0, _check_dest) try: diff --git a/tests/test_subcmds_upload.py b/tests/test_subcmds_upload.py index 8968042b8..a91c7456e 100644 --- a/tests/test_subcmds_upload.py +++ b/tests/test_subcmds_upload.py @@ -14,7 +14,7 @@ """Unittests for the subcmds/upload.py module.""" -from typing import List, Optional +from typing import Iterator, List, Optional from unittest import mock import pytest @@ -34,7 +34,7 @@ _STUB_PEOPLE = ([], []) @pytest.fixture -def cmd() -> upload.Upload: +def cmd() -> Iterator[upload.Upload]: """Fixture to provide an Upload command instance with mocked methods.""" cmd = upload.Upload() with mock.patch.object( diff --git a/tests/utils_for_test.py b/tests/utils_for_test.py index 3f1bed486..18d36ad23 100644 --- a/tests/utils_for_test.py +++ b/tests/utils_for_test.py @@ -50,7 +50,7 @@ def init_git_tree( else: # Use template dir for init. templatedir = stack.enter_context( - tempfile.mkdtemp(prefix="git-template") + tempfile.TemporaryDirectory(prefix="git-template") ) (Path(templatedir) / "HEAD").write_text("ref: refs/heads/main\n") cmd += ["--template", templatedir] diff --git a/wrapper.py b/wrapper.py index 55082248f..601a64efa 100644 --- a/wrapper.py +++ b/wrapper.py @@ -31,6 +31,7 @@ def Wrapper(): modname = "wrapper" loader = importlib.machinery.SourceFileLoader(modname, WrapperPath()) spec = importlib.util.spec_from_loader(modname, loader) + assert spec is not None module = importlib.util.module_from_spec(spec) loader.exec_module(module) return module