New helper script to run fast lints and pre-push hook that runs it #3949

jleibs · 2023-10-21T00:42:13Z

What

Resolves:

Pre-push git hook that checks for typos and runs lint.py #3738

Builds out a micro-framework for running a bunch of lint-jobs in parallel as part of a local development cycle. Anything that we can run in a few seconds that would otherwise trip up CI should be a good contender for adding to this list.

Takes advantage of the diff relative to the common ancestor to only check modified files.

Also introduces a hooks directory for running as part of a pre-push hook.

Examples:

$ just fast-lint 
pixi run python scripts/fast_lint.py 
INFO:root:SKIP: lint-cpp-files (no modified files)
INFO:root:SKIP: lint-rs-files (no modified files)
INFO:root:PASS: lint-taplo in 0.27s
INFO:root:PASS: lint-py-ruff in 0.28s
INFO:root:PASS: lint-typos in 0.29s
INFO:root:PASS: lint-py-black in 0.38s
INFO:root:PASS: lint-rerun in 0.39s
INFO:root:PASS: lint-py-mypy in 0.48s
INFO:root:PASS: lint-py-blackdoc in 0.48s
INFO:root:PASS: lint-codegen in 3.53s
INFO:root:All lints passed in 3.54s

Optionally skip some lints:

pixi run python scripts/fast_lint.py --skip lint-codegen
INFO:root:SKIP: lint-codegen (skipped manually)
INFO:root:SKIP: lint-cpp-files (no modified files)
INFO:root:SKIP: lint-rs-files (no modified files)
INFO:root:PASS: lint-taplo in 0.27s
INFO:root:PASS: lint-py-ruff in 0.28s
INFO:root:PASS: lint-typos in 0.30s
INFO:root:PASS: lint-rerun in 0.42s
INFO:root:PASS: lint-py-black in 0.43s
INFO:root:PASS: lint-py-blackdoc in 0.44s
INFO:root:PASS: lint-py-mypy in 0.62s
INFO:root:All lints passed in 0.63s

Or force lints to run on the full tree without the change filters:

$ just fast-lint --no-change-filter
pixi run python scripts/fast_lint.py --no-change-filter
INFO:root:SKIP: lint-cpp-files (no-change-filter not supported)
INFO:root:PASS: lint-py-ruff in 0.29s
INFO:root:PASS: lint-py-blackdoc in 0.64s
INFO:root:PASS: lint-py-mypy in 0.75s
INFO:root:PASS: lint-typos in 1.44s
INFO:root:PASS: lint-taplo in 1.77s
INFO:root:PASS: lint-py-black in 2.13s
INFO:root:PASS: lint-rs-all in 3.89s
INFO:root:PASS: lint-codegen in 4.92s
INFO:root:PASS: lint-rerun in 5.43s
INFO:root:All lints passed in 5.43s

Checklist

I have read and agree to Contributor Guide and the Code of Conduct
I've included a screenshot or gif (if applicable)
I have tested demo.rerun.io (if applicable)
The PR title and labels are set such as to maximize their usefulness for the next release's CHANGELOG

abey79

This is such a great addition, I love it!

Two things:

codegen check is on the slow side for a pre-commit hook
there should be a note about this in CONTRIBUTING.md (and elsewhere?), ideally along with a short reminder on how to add it as pre-commit

scripts/fast_lint.py

abey79 · 2023-10-21T08:58:10Z

scripts/fast_lint.py

+        (["rustfmt", "--check"], filter_ext(files, [".rs"])),
+        (["python", "scripts/lint.py"], files),
+        (["typos"], files),
+        (["taplo", "fmt", "--check"], filter_ext(files, [".toml"])),


Love this so much! Taplo is way to slow when running `just toml-lint.

abey79 · 2023-10-21T09:05:56Z

scripts/fast_lint.py

+        logging.debug(f"  {f}")
+
+    jobs = [
+        (["pixi", "run", "codegen", "--check"], True),


This one takes 5-7s for me, which is dangerously close to some threshold as far as pre-commit hook are concerned.

❯ python scripts/fast_lint.py INFO:root:SKIP: `rustfmt --check` INFO:root:SKIP: `taplo fmt --check` INFO:root:SKIP: `pixi run clang-format --dry-run` INFO:root:PASS: `ruff check --config rerun_py/pyproject.toml <FILES>` in 0.01s INFO:root:PASS: `typos <FILES>` in 0.04s INFO:root:PASS: `python scripts/lint.py <FILES>` in 0.10s INFO:root:PASS: `blackdoc --check <FILES>` in 0.51s INFO:root:PASS: `black --check --config rerun_py/pyproject.toml <FILES>` in 0.54s INFO:root:FAIL: `mypy --no-warn-unused-ignore <FILES>` in 0.99s INFO:root:stdout: scripts/screenshot_compare/build_screenshot_compare.py:84: error: Incompatible types in assignment (expression has type "str", variable has type "Path") [assignment] Found 1 error in 1 file (checked 2 source files) INFO:root:PASS: `pixi run codegen --check ` in 5.70s INFO:root:Lints failed in 5.73s

That's after a few runs to warm things up.

(running on my antoine/screenshot_compare branch)

as far as pre-commi

Ahh yeah, agreed it's slow for a pre-commit. I have been primarily thinking of this as a pre-push, which I think strikes the right balance. I've now added option to skip arbitrary jobs if you wanted to use for a pre-commit as well.

pre-push makes sense, yeah 👍🏻

Still sounds too slow for a pre-push for my taste. It's also not something that I usually forget to do (running codegen), while things like typos fail my CI very often.

How often are you git pushing?

I dunno. 5-20 times a day? 🤷

Alright -- if you want to save those 25 - 100 seconds you can now:

export RERUN_LINT_SKIP=lint-codegen

scripts/fast_lint.py

abey79 · 2023-10-21T09:16:20Z

scripts/fast_lint.py

+        (["black", "--check", "--config", "rerun_py/pyproject.toml"], filter_ext(files, [".py"])),
+        (["blackdoc", "--check"], filter_ext(files, [".py"])),
+        (["mypy", "--no-warn-unused-ignore"], filter_ext(files, [".py"])),
+    ]


Out of this PR scope, but it would be nice if all of these would actually be (just_command, file_list) tuple. justfile as single source of truth on how to run things...

I'm not totally sold on making these all dispatch through just since I'm not sure running them individually always makes sense. However, I've added them all to pixi and updated the pixi deps so they should now run deterministically from the pixi environment.

…ture

…then switching branches

abey79 · 2023-10-22T08:48:33Z

hooks/README.md

+or if you prefer you can configure git to use this directory as the hooks directory:
+```
+git config core.hooksPath hooks
+```


emilk · 2023-10-23T07:26:12Z

CONTRIBUTING.md

+To install the hooks, simply copy them into the `.git/hooks` directory of your local checkout.
+```
+cp hooks/pre-push .git/hooks/pre-push
+chmod +x .git/hooks/pre-push


why not chmod +x hooks/pre-push already?

It generally should be, though I've definitely seen edge-cases across OS's and git configurations where that isn't guaranteed.

jleibs added 2 commits October 21, 2023 02:32

Initial scaffolding for a fast linter

ff658b4

Run a bunch more lints

785e5e4

jleibs added the 🧑‍💻 dev experience developer experience (excluding CI) label Oct 21, 2023

jleibs marked this pull request as ready for review October 21, 2023 00:42

jleibs added the exclude from changelog PRs with this won't show up in CHANGELOG.md label Oct 21, 2023

jleibs added 2 commits October 21, 2023 02:45

Cleanup

ebc8fcf

Add to justfile

2edc548

jleibs force-pushed the jleibs/fast_lint branch from ba6eef8 to 2edc548 Compare October 21, 2023 00:53

Fix print on SKIP

c7d4431

abey79 approved these changes Oct 21, 2023

View reviewed changes

jleibs added 4 commits October 21, 2023 16:42

Assorted improvements

012549d

Cleanup and support --no-file-filter

218a3e4

Renames

19b07e5

Improve skip messages

5669a09

jleibs force-pushed the jleibs/fast_lint branch from 2ceb454 to 5669a09 Compare October 21, 2023 16:39

jleibs added 4 commits October 21, 2023 18:56

Add a bit to contributing

8230e9d

Add a managed hooks folder and installation instructions

a7bb52c

Check for pixi

3652137

Pixi target for fast-lint

664702b

jleibs force-pushed the jleibs/fast_lint branch from d421791 to 664702b Compare October 21, 2023 17:30

jleibs changed the title ~~New helper script to run lints based on changes local to the branch~~ New helper script to run fast lints and github hooks directory. Oct 21, 2023

jleibs changed the title ~~New helper script to run fast lints and github hooks directory.~~ New helper script to run fast lints and pre-push hook that runs it Oct 21, 2023

jleibs added include in changelog and removed exclude from changelog PRs with this won't show up in CHANGELOG.md labels Oct 21, 2023

jleibs added 3 commits October 21, 2023 19:40

Pass through git hook arguments in case we want to use them in the fu…

7681b79

…ture

Use exec so we have access to stdin

3a23cf9

Only run fast-lint if the pushed branch is active

e1b2bd7

jleibs force-pushed the jleibs/fast_lint branch from 672001c to e1b2bd7 Compare October 21, 2023 17:57

jleibs added 2 commits October 21, 2023 20:04

Don't fail if the hook is missing -- such as installing via copy and …

eb8fee5

…then switching branches

Convert indentation to spaces

20f52fd

jleibs added 2 commits October 21, 2023 20:18

Rename the root logger

76f39da

Re-format log output

7fbed6f

jleibs force-pushed the jleibs/fast_lint branch from eea25dd to 7fbed6f Compare October 21, 2023 18:21

abey79 reviewed Oct 22, 2023

View reviewed changes

emilk reviewed Oct 23, 2023

View reviewed changes

emilk and others added 2 commits October 23, 2023 09:32

Check that the commands in --skip are real commands

52fd652

Add environment variable for skipping lints

6a955f8

jleibs merged commit 8e57f41 into main Oct 23, 2023
32 checks passed

jleibs deleted the jleibs/fast_lint branch October 23, 2023 16:24

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

New helper script to run fast lints and pre-push hook that runs it #3949

New helper script to run fast lints and pre-push hook that runs it #3949

jleibs commented Oct 21, 2023 •

edited by github-actions bot

Loading

abey79 left a comment

abey79 Oct 21, 2023

abey79 Oct 21, 2023

abey79 Oct 21, 2023

jleibs Oct 21, 2023

abey79 Oct 21, 2023

emilk Oct 22, 2023

jleibs Oct 22, 2023

emilk Oct 23, 2023

jleibs Oct 23, 2023

abey79 Oct 21, 2023

jleibs Oct 21, 2023

abey79 Oct 22, 2023

emilk Oct 23, 2023

jleibs Oct 23, 2023

New helper script to run fast lints and pre-push hook that runs it #3949

New helper script to run fast lints and pre-push hook that runs it #3949

Conversation

jleibs commented Oct 21, 2023 • edited by github-actions bot Loading

What

Checklist

abey79 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jleibs commented Oct 21, 2023 •

edited by github-actions bot

Loading