Rework our linting setup (#11522)

Co-authored-by: Avasam <[email protected]>

Author: AlexWaygood

.github/workflows/tests.yml

@@ -44,27 +44,6 @@ jobs:
           python-version: "3.11"
       - run: ./tests/check_new_syntax.py
 
-  ruff:
-    name: Lint with Ruff
-    runs-on: ubuntu-latest
-    steps:
-      - uses: actions/checkout@v4
-      - uses: chartboost/ruff-action@v1
-        with:
-          version: "0.2.1" # must match .pre-commit-config.yaml and requirements-test.txt
-
-  flake8:
-    name: Lint with Flake8
-    runs-on: ubuntu-latest
-    steps:
-      - uses: actions/checkout@v4
-      - uses: actions/setup-python@v4
-        with:
-          python-version: "3.11"
-      - run: curl -LsSf https://astral.sh/uv/install.sh | sh
-      - run: uv pip install -r requirements-tests.txt --system
-      - run: flake8 --color always
-
   pytype:
     name: Run pytype against the stubs
     runs-on: ubuntu-latest

.pre-commit-config.yaml

@@ -8,17 +8,30 @@ repos:
       - id: check-toml
       - id: check-merge-conflict
       - id: mixed-line-ending
+        args: [--fix=lf]
       - id: check-case-conflict
+  - repo: https://github.com/astral-sh/ruff-pre-commit
+    rev: v0.3.0 # must match requirements-tests.txt
+    hooks:
+      - id: ruff
+        name: Run ruff on stubs, tests and scripts
+        args: ["--exit-non-zero-on-fix"]
+      - id: ruff
+        # Run this separately because we don't really want
+        # to use --unsafe-fixes for all rules
+        name: Remove unnecessary `sys.version_info` blocks
+        args: ["--exit-non-zero-on-fix", "--select=UP036", "--unsafe-fixes"]
+      - id: ruff
+        # Very few rules are useful to run on our test cases;
+        # we explicitly enumerate them here:
+        name: Run ruff on the test cases
+        args: ["--exit-non-zero-on-fix", "--select=FA,I", "--no-force-exclude", "--unsafe-fixes"]
+        files: '.*test_cases/.+\.py$'
   - repo: https://github.com/psf/black-pre-commit-mirror
     rev: 24.1.1 # must match requirements-tests.txt
     hooks:
       - id: black
         language_version: python3.10
-  - repo: https://github.com/astral-sh/ruff-pre-commit
-    rev: v0.2.1 # must match requirements-tests.txt and tests.yml
-    hooks:
-      - id: ruff
-        args: [--exit-non-zero-on-fix, --fix-only]
   - repo: https://github.com/pycqa/flake8
     rev: 7.0.0 # must match requirements-tests.txt
     hooks:
@@ -28,11 +41,13 @@ repos:
           - "flake8-pyi==24.1.0" # must match requirements-tests.txt
         types: [file]
         types_or: [python, pyi]
+  - repo: meta
+    hooks:
+      - id: check-hooks-apply
 
 ci:
   autofix_commit_msg: "[pre-commit.ci] auto fixes from pre-commit.com hooks"
   autofix_prs: true
   autoupdate_commit_msg: "[pre-commit.ci] pre-commit autoupdate"
   autoupdate_schedule: quarterly
-  skip: [flake8]
   submodules: false

CONTRIBUTING.md

@@ -87,34 +87,25 @@ terminal to install all non-pytype requirements:
 ## Code formatting
 
 The code is formatted using [`Black`](https://github.com/psf/black).
-Various other autofixes are
-also performed by [`Ruff`](https://github.com/astral-sh/ruff).
+Various other autofixes and lint rules are
+also performed by [`Ruff`](https://github.com/astral-sh/ruff) and
+[`Flake8`](https://github.com/pycqa/flake8),
+with plugins [`flake8-pyi`](https://github.com/pycqa/flake8-pyi),
+and [`flake8-noqa`](https://github.com/plinss/flake8-noqa).
 
 The repository is equipped with a [`pre-commit.ci`](https://pre-commit.ci/)
 configuration file. This means that you don't *need* to do anything yourself to
-run the code formatters. When you push a commit, a bot will run those for you
-right away and add a commit to your PR.
-
-That being said, if you *want* to run the checks locally when you commit,
-you're free to do so. Either run the following manually...
+run the code formatters or linters. When you push a commit, a bot will run
+those for you right away and add any autofixes to your PR. Anything
+that can't be autofixed will show up as a CI failure, hopefully with an error
+message that will make it clear what's gone wrong.
 
-```bash
-(.venv)$ ruff check .
-(.venv)$ black .
-```
-
-...Or install the pre-commit hooks: please refer to the
-[pre-commit](https://pre-commit.com/) documentation.
-
-Our code is also linted using [`Flake8`](https://github.com/pycqa/flake8),
-with plugins [`flake8-pyi`](https://github.com/pycqa/flake8-pyi),
-and [`flake8-noqa`](https://github.com/plinss/flake8-noqa).
-As with our other checks, running
-Flake8 before filing a PR is not required. However, if you wish to run Flake8
-locally, install the test dependencies as outlined above, and then run:
+That being said, if you *want* to run the formatters and linters locally
+when you commit, you're free to do so. To use the same configuration as we use
+in CI, we recommend doing this via pre-commit:
 
 ```bash
-(.venv)$ flake8 .
+(.venv)$ pre-commit run --all-files
 ```
 
 ## Where to make changes

pyproject.toml

@@ -24,6 +24,14 @@ exclude = [
 ]
 
 [tool.ruff.lint]
+# Disable all rules on test cases by default:
+# test cases often deliberately contain code
+# that might not be considered idiomatic or modern.
+#
+# Note: some rules that are specifically useful to the test cases
+# are invoked via separate runs of ruff in pre-commit:
+# see our .pre-commit-config.yaml file for details
+exclude = ["**/test_cases/**/*.py"]
 select = [
     "B", # flake8-bugbear
     "FA", # flake8-future-annotations
@@ -48,12 +56,8 @@ ignore = [
 ]
 
 [tool.ruff.lint.per-file-ignores]
-# Disable "modernization" rules with autofixes from test cases as they often
-# deliberately contain code that might not be considered idiomatic or modern
-# These can be run manually once in a while
-"**/test_cases/**/*.py" = ["UP"]
 "*.pyi" = [
-    # Most flake8-bugbear rules don't apply for third-party stubs like typeshed,
+    # Most flake8-bugbear rules don't apply for third-party stubs like typeshed.
     # B033 could be slightly useful but Ruff doesn't have per-file select
     "B", # flake8-bugbear
 ]

requirements-tests.txt

@@ -8,12 +8,13 @@ flake8-pyi==24.1.0       # must match .pre-commit-config.yaml
 mypy==1.8.0
 pre-commit-hooks==4.5.0  # must match .pre-commit-config.yaml
 pytype==2024.2.27; platform_system != "Windows" and python_version < "3.12"
-ruff==0.2.1              # must match .pre-commit-config.yaml and tests.yml
+ruff==0.3.0              # must match .pre-commit-config.yaml
 
 # Libraries used by our various scripts.
 aiohttp==3.9.3
 packaging==23.2
 pathspec>=0.11.1
+pre-commit
 pyyaml==6.0.1
 stubdefaulter==0.1.0
 termcolor>=2.3

scripts/generate_proto_stubs.sh

@@ -45,10 +45,7 @@ PYTHON_PROTOBUF_DIR="protobuf-$PYTHON_PROTOBUF_VERSION"
 VENV=venv
 python3 -m venv "$VENV"
 source "$VENV/bin/activate"
-pip install -r "$REPO_ROOT/requirements-tests.txt"  # for Black and Ruff
-
-# Install mypy-protobuf
-pip install mypy-protobuf=="$MYPY_PROTOBUF_VERSION"
+pip install pre-commit mypy-protobuf=="$MYPY_PROTOBUF_VERSION"
 
 # Remove existing pyi
 find "$REPO_ROOT/stubs/protobuf/" -name '*_pb2.pyi' -delete
@@ -73,9 +70,9 @@ PROTO_FILES=$(grep "GenProto.*google" $PYTHON_PROTOBUF_DIR/python/setup.py | \
 # shellcheck disable=SC2086
 protoc_install/bin/protoc --proto_path="$PYTHON_PROTOBUF_DIR/src" --mypy_out="relax_strict_optional_primitives:$REPO_ROOT/stubs/protobuf" $PROTO_FILES
 
-ruff check "$REPO_ROOT/stubs/protobuf" --fix-only
-ruff check "$REPO_ROOT/stubs/protobuf" --fix-only --select=UP036 --unsafe-fixes
-black "$REPO_ROOT/stubs/protobuf"
+# use `|| true` so the script still continues even if a pre-commit hook
+# applies autofixes (which will result in a nonzero exit code)
+pre-commit run --files "$REPO_ROOT/stubs/protobuf" || true
 
 sed --in-place="" \
   "s/extra_description = .*$/extra_description = \"Generated using [mypy-protobuf==$MYPY_PROTOBUF_VERSION](https:\/\/github.com\/nipunn1313\/mypy-protobuf\/tree\/v$MYPY_PROTOBUF_VERSION) on protobuf==$PYTHON_PROTOBUF_VERSION\"/" \

scripts/runtests.py

@@ -81,17 +81,8 @@ def main() -> None:
     stubtest_result: subprocess.CompletedProcess[bytes] | None = None
     pytype_result: subprocess.CompletedProcess[bytes] | None = None
 
-    # Run formatters first. Order matters.
-    print("\nRunning Ruff...")
-    subprocess.run([sys.executable, "-m", "ruff", "check", path])
-    print("\nRunning Black...")
-    black_result = subprocess.run([sys.executable, "-m", "black", path])
-    if black_result.returncode == 123:
-        print("Could not run tests due to an internal error with Black. See above for details.", file=sys.stderr)
-        sys.exit(black_result.returncode)
-
-    print("\nRunning Flake8...")
-    flake8_result = subprocess.run([sys.executable, "-m", "flake8", path])
+    print("\nRunning pre-commit...")
+    pre_commit_result = subprocess.run(["pre-commit", "run", "--all-files"])
 
     print("\nRunning check_consistent.py...")
     check_consistent_result = subprocess.run([sys.executable, "tests/check_consistent.py"])
@@ -187,7 +178,7 @@ def main() -> None:
 
     any_failure = any(
         [
-            flake8_result.returncode,
+            pre_commit_result.returncode,
             check_consistent_result.returncode,
             check_new_syntax_result.returncode,
             pyright_returncode,
@@ -203,7 +194,18 @@ def main() -> None:
         print(colored("\n\n--- TEST SUMMARY: One or more tests failed. See above for details. ---\n", "red"))
     else:
         print(colored("\n\n--- TEST SUMMARY: All tests passed! ---\n", "green"))
-    print("Flake8:", _SUCCESS if flake8_result.returncode == 0 else _FAILED)
+    if pre_commit_result.returncode == 0:
+        print("pre-commit", _SUCCESS)
+    else:
+        print("pre-commit", _FAILED)
+        print(
+            """\
+  Check the output of pre-commit for more details.
+  This could mean that there's a lint failure on your code,
+  but could also just mean that one of the pre-commit tools
+  applied some autofixes. If the latter, you may want to check
+  that the autofixes did sensible things."""
+        )
     print("Check consistent:", _SUCCESS if check_consistent_result.returncode == 0 else _FAILED)
     print("Check new syntax:", _SUCCESS if check_new_syntax_result.returncode == 0 else _FAILED)
     if pyright_skipped:

scripts/sync_tensorflow_protobuf_stubs.sh

@@ -15,7 +15,7 @@ TENSORFLOW_VERSION=2.12.1
 # protobuf<4.
 MYPY_PROTOBUF_VERSION=3.5.0
 
-pip install mypy-protobuf=="$MYPY_PROTOBUF_VERSION"
+pip install pre-commit mypy-protobuf=="$MYPY_PROTOBUF_VERSION"
 
 cd "$(dirname "$0")" > /dev/null
 cd ../stubs/tensorflow
@@ -61,9 +61,9 @@ rm tensorflow/compiler/xla/service/hlo_execution_profile_data_pb2.pyi \
    tensorflow/core/protobuf/worker_service_pb2.pyi \
    tensorflow/core/util/example_proto_fast_parsing_test_pb2.pyi
 
-ruff check "$REPO_ROOT/stubs/tensorflow/tensorflow" --fix-only
-ruff check "$REPO_ROOT/stubs/protobuf" --fix-only --select=UP036 --unsafe-fixes
-black "$REPO_ROOT/stubs/tensorflow/tensorflow"
+# use `|| true` so the script still continues even if a pre-commit hook
+# applies autofixes (which will result in a nonzero exit code)
+pre-commit run --files "$REPO_ROOT/stubs/tensorflow/tensorflow" || true
 
 sed --in-place="" \
     "s/extra_description = .*$/extra_description = \"Partially generated using [mypy-protobuf==$MYPY_PROTOBUF_VERSION](https:\/\/github.com\/nipunn1313\/mypy-protobuf\/tree\/v$MYPY_PROTOBUF_VERSION) on tensorflow==$TENSORFLOW_VERSION\"/" \

tests/check_consistent.py

@@ -145,7 +145,6 @@ def get_txt_requirements() -> dict[str, SpecifierSet]:
 class PreCommitConfigRepos(TypedDict):
     hooks: list[dict[str, str]]
     repo: str
-    rev: str
 
 
 class PreCommitConfig(TypedDict):
@@ -158,16 +157,16 @@ def get_precommit_requirements() -> dict[str, SpecifierSet]:
     yam: PreCommitConfig = yaml.load(precommit, Loader=yaml.Loader)
     precommit_requirements: dict[str, SpecifierSet] = {}
     for repo in yam["repos"]:
-        if not repo.get("python_requirement", True):
+        package_rev = repo.get("rev")
+        if not isinstance(package_rev, str):
             continue
-        hook = repo["hooks"][0]
         package_name = Path(urllib.parse.urlparse(repo["repo"]).path).name
-        package_rev = repo["rev"].removeprefix("v")
-        package_specifier = SpecifierSet(f"=={package_rev}")
+        package_specifier = SpecifierSet(f"=={package_rev.removeprefix('v')}")
         precommit_requirements[package_name] = package_specifier
-        for additional_req in hook.get("additional_dependencies", ()):
-            req = Requirement(additional_req)
-            precommit_requirements[req.name] = req.specifier
+        for hook in repo["hooks"]:
+            for additional_req in hook.get("additional_dependencies", ()):
+                req = Requirement(additional_req)
+                precommit_requirements[req.name] = req.specifier
     return precommit_requirements