From 5c416e4d9b67635be7b18b6e03e5151d6c446ef8 Mon Sep 17 00:00:00 2001 From: konstin Date: Sun, 18 Jun 2023 13:00:42 +0200 Subject: [PATCH] Pre commit without cargo and other pre-PR improvements (#5146) This tackles three problems: * pre-commit was slow because it ran cargo commands * Improve the clarity on what you need to run to get your PR pass on CI (and make those fast) * You had to compile and run `cargo dev generate-all` separately, which was slow The first change is to remove all cargo commands except running ruff itself from pre-commit. With `cargo run --bin ruff` already compiled it takes about 7s on my machine. It would make sense to also use the ruff pre-commit action here even if we're then lagging a release behind for checking ruff on ruff. The contributing guide is now clear about what you need to run: ```shell cargo clippy --workspace --all-targets --all-features -- -D warnings # Linting... RUFF_UPDATE_SCHEMA=1 cargo test # Testing and updating ruff.schema.json pre-commit run --all-files # rust and python formatting, markdown and python linting, etc. ``` Example timings from my machine: `cargo clippy --workspace --all-targets --all-features -- -D warnings`: 23s `RUFF_UPDATE_SCHEMA=1 cargo test`: 2min (recompiling), 1min (no code changes, this is mainly doc tests) `pre-commit run --all-files`: 7s The exact numbers don't matter so much as the approximate experience (6s is easier to just wait than 1min, esp if you need to fix and rerun). The biggest remaining block seems to be doc tests, i'm surprised i didn't find any solution to speeding them up (nextest simply doesn't run them at all). Also note that the formatter has it's own tests which are much faster since they avoid linking ruff (`cargo test ruff_python_formatter`). The third change is to enable `cargo test` to update the schema. Similar to `INSTA_UPDATE=always`, i've added `RUFF_UPDATE_SCHEMA=1` (name open to bikeshedding), so `RUFF_UPDATE_SCHEMA=1 cargo test` updates the schema, while `cargo test` still fails as expected if the repo isn't up-to-date. --------- Co-authored-by: Dhruv Manilawala --- .pre-commit-config.yaml | 20 +++++--------------- CONTRIBUTING.md | 19 +++++++++---------- crates/ruff_dev/src/generate_json_schema.rs | 19 +++++++++++++++---- 3 files changed, 29 insertions(+), 29 deletions(-) diff --git a/.pre-commit-config.yaml b/.pre-commit-config.yaml index d89b927114..16ffe82286 100644 --- a/.pre-commit-config.yaml +++ b/.pre-commit-config.yaml @@ -38,29 +38,19 @@ repos: name: cargo fmt entry: cargo fmt -- language: system - types: [rust] - - id: clippy - name: clippy - entry: cargo clippy --workspace --all-targets --all-features -- -D warnings - language: system - pass_filenames: false + types: [ rust ] + pass_filenames: false # This makes it a lot faster - id: ruff name: ruff - entry: cargo run -p ruff_cli -- check --no-cache --force-exclude --fix --exit-non-zero-on-fix + entry: cargo run --bin ruff -- check --no-cache --force-exclude --fix --exit-non-zero-on-fix language: system - types_or: [python, pyi] + types_or: [ python, pyi ] require_serial: true exclude: | (?x)^( crates/ruff/resources/.*| crates/ruff_python_formatter/resources/.* )$ - - id: dev-generate-all - name: dev-generate-all - entry: cargo dev generate-all - language: system - pass_filenames: false - exclude: target # Black - repo: https://github.com/psf/black @@ -69,4 +59,4 @@ repos: - id: black ci: - skip: [cargo-fmt, clippy, dev-generate-all] + skip: [ cargo-fmt, dev-generate-all ] diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md index 8ad067e335..3feb49bd1f 100644 --- a/CONTRIBUTING.md +++ b/CONTRIBUTING.md @@ -45,6 +45,12 @@ You'll also need [Insta](https://insta.rs/docs/) to update snapshot tests: cargo install cargo-insta ``` +and pre-commit to run some validation checks: + +```shell +pipx install pre-commit # or `pip install pre-commit` if you have a virtualenv +``` + ### Development After cloning the repository, run Ruff locally with: @@ -57,9 +63,9 @@ Prior to opening a pull request, ensure that your code has been auto-formatted, and that it passes both the lint and test validation checks: ```shell -cargo fmt # Auto-formatting... -cargo test # Testing... -cargo clippy --workspace --all-targets --all-features -- -D warnings # Linting... +cargo clippy --workspace --all-targets --all-features -- -D warnings # Rust linting +RUFF_UPDATE_SCHEMA=1 cargo test # Rust testing and updating ruff.schema.json +pre-commit run --all-files --show-diff-on-failure # Rust and Python formatting, Markdown and Python linting, etc. ``` These checks will run on GitHub Actions when you open your Pull Request, but running them locally @@ -72,13 +78,6 @@ after running `cargo test` like so: cargo insta review ``` -If you have `pre-commit` [installed](https://pre-commit.com/#installation) then you can use it to -assist with formatting and linting. The following command will run the `pre-commit` hooks: - -```shell -pre-commit run --all-files -``` - Your Pull Request will be reviewed by a maintainer, which may involve a few rounds of iteration prior to merging. diff --git a/crates/ruff_dev/src/generate_json_schema.rs b/crates/ruff_dev/src/generate_json_schema.rs index 147638dba6..046b917294 100644 --- a/crates/ruff_dev/src/generate_json_schema.rs +++ b/crates/ruff_dev/src/generate_json_schema.rs @@ -32,15 +32,20 @@ pub(crate) fn main(args: &Args) -> Result<()> { Mode::Check => { let current = fs::read_to_string(schema_path)?; if current == schema_string { - println!("up-to-date: {filename}"); + println!("Up-to-date: {filename}"); } else { let comparison = StrComparison::new(¤t, &schema_string); bail!("{filename} changed, please run `{REGENERATE_ALL_COMMAND}`:\n{comparison}"); } } Mode::Write => { - let file = schema_path; - fs::write(file, schema_string.as_bytes())?; + let current = fs::read_to_string(&schema_path)?; + if current == schema_string { + println!("Up-to-date: {filename}"); + } else { + println!("Updating: {filename}"); + fs::write(schema_path, schema_string.as_bytes())?; + } } } @@ -50,6 +55,7 @@ pub(crate) fn main(args: &Args) -> Result<()> { #[cfg(test)] mod test { use anyhow::Result; + use std::env; use crate::generate_all::Mode; @@ -57,6 +63,11 @@ mod test { #[test] fn test_generate_json_schema() -> Result<()> { - main(&Args { mode: Mode::Check }) + let mode = if env::var("RUFF_UPDATE_SCHEMA").as_deref() == Ok("1") { + Mode::Write + } else { + Mode::Check + }; + main(&Args { mode }) } }