diff --git a/.github/workflows/tests.yml b/.github/workflows/tests.yml index e6ed3481c..b2453af4e 100644 --- a/.github/workflows/tests.yml +++ b/.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 diff --git a/.pre-commit-config.yaml b/.pre-commit-config.yaml index 9bf998ec4..dc4bc138b 100644 --- a/.pre-commit-config.yaml +++ b/.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 diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md index 1dd7fd6fd..93e0626f8 100644 --- a/CONTRIBUTING.md +++ b/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. +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. -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... +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)$ 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: - -```bash -(.venv)$ flake8 . +(.venv)$ pre-commit run --all-files ``` ## Where to make changes diff --git a/pyproject.toml b/pyproject.toml index 64a5cda84..9239e77a5 100644 --- a/pyproject.toml +++ b/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 ] diff --git a/requirements-tests.txt b/requirements-tests.txt index b336a4508..cf3bbef95 100644 --- a/requirements-tests.txt +++ b/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 diff --git a/scripts/generate_proto_stubs.sh b/scripts/generate_proto_stubs.sh index 0e10aa2b4..88c36bef7 100755 --- a/scripts/generate_proto_stubs.sh +++ b/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\"/" \ diff --git a/scripts/runtests.py b/scripts/runtests.py index f8c1ce8f2..4e7755bc6 100755 --- a/scripts/runtests.py +++ b/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: diff --git a/scripts/sync_tensorflow_protobuf_stubs.sh b/scripts/sync_tensorflow_protobuf_stubs.sh index 3e4a355e0..29b603c81 100755 --- a/scripts/sync_tensorflow_protobuf_stubs.sh +++ b/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\"/" \ diff --git a/tests/check_consistent.py b/tests/check_consistent.py index 1abb4d635..bf02d848c 100755 --- a/tests/check_consistent.py +++ b/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