Rework our linting setup #11522
Rework our linting setup #11522
Conversation
Co-authored-by: Avasam <samuel.06@hotmail.com>
tests/check_consistent.py
Outdated
There was a problem hiding this comment.
The changes to this file were necessary because the test currently assumes that all repos listed in .pre-commit-config.yaml will have a rev key in them, and that's not true for the "meta" repo that I'm adding as part of this PR
scripts/runtests.py
Outdated
| @@ -203,7 +194,7 @@ 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) | |||
| print("pre-commit:", _SUCCESS if pre_commit_result.returncode == 0 else _FAILED) | |||
There was a problem hiding this comment.
If black formats a file, or Ruff autofixes something with no issues left, this shows up as failed. It's not major, but it is a change of behaviour.
Would it matter for the CI if black only fails on unformattable / syntax errors, and Ruff on leftover issues ? Since a change (format/autofix) retriggers the CI anyway iirc.
Also from CONTRIBUTING.md
Anything that can't be autofixed will show up as a CI failure
idk how this should interact with git pre-commit hooks if pre-commit is installed as one, given I don't use 'em.
There was a problem hiding this comment.
Good point. I think it's useful for black and ruff to give nonzero exit codes when they do autofixes, because it's sometimes important to manually review the output of the autofixes before making the PR. What do you think of 6f079ee?
idk how this should interact with git pre-commit hooks if
pre-commitis installed as one, given I don't use 'em.
¯\_(ツ)_/¯ (I don't use them either 😄)
There was a problem hiding this comment.
Example output from runtests.py if pre-commit fails, following 6f079ee:
There was a problem hiding this comment.
I think it's useful for black and ruff to give nonzero exit codes when they do autofixes, because it's sometimes important to manually review the output of the autofixes before making the PR. What do you think of 6f079ee?
I think that's valid and something we already could've done by showing a warning instead of passing, for example.
Now with pre-commit, the next best thing is probably failing with a message like you did.
One tiny nitpick is that Check the output of pre-commit for more details. might be overly verbose given One or more tests failed. See above for details. is written two lines above. But that won't block my approval
Edit: Sorry but I find it really funny to see the "how to screenshot on mac" page in the background. Started my day with a good laugh 😄
There was a problem hiding this comment.
Alex finding out the hard way that Terminal is transparent on Macs...
There was a problem hiding this comment.
WHY CAN'T THERE BE A BUTTON ON THE KEYBOARD FOR PRINT-SCREENING LIKE ON WINDOWS
Co-authored-by: Avasam <samuel.06@hotmail.com>
…into pre-commit
There was a problem hiding this comment.
This all LGTM. I've used pre-commit as the CLI command for linting/formatting in some project, and I don't see an issue using it more that way in typeshed.
All my concerns have been addressed though I may want to hear from another maintainer. Otherwise happy to merge.
|
Thanks, both! |
ruffversion pin in 2 places, rather than 3--unsafe-fixesfor one specific rulecheck-hooks-apply. This will error out if a regex passed to thefilesparameter for any individual pre-commit hook matches 0 filesI didn't succeed in running the protobuf-stub-generation scripts locally on my MacBook (something about
realpathnot accepting an-L1argument on the MacOS version?), so please review the changes to those files carefully :)