🌐 AI搜索 & 代理 主页
Skip to content
Prev Previous commit
Next Next commit
Try pre-commit with '--check'
  • Loading branch information
AA-Turner committed Jul 19, 2024
commit 933095c705a91b0dc1b864c1c4294766b96ad587
2 changes: 1 addition & 1 deletion .pre-commit-config.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,7 @@ repos:
files: ^Tools/clinic/|Lib/test/test_clinic.py
- id: ruff-format
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@AA-Turner in pre-commit, it's usually best to put formatters before linters. This is because if there's a rule violation that linter shows, it will then show up as two different broken checks. It's also more important for the post-format version to be checked rather than showing errors on things that then get changed/relocated.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh, are the pre-commit tools not idempotent? I hadn't realised that order mattered, as I thought we'd configured them to check only in a read-only fashion.

As you can probably tell, I don't use pre-commit personally!

A

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The Ruff pre-commit README says:

When running with --fix, Ruff's lint hook should be placed before Ruff's formatter hook, and before Black, isort, and other formatting tools, as Ruff's fix behavior can output code changes that require reformatting.

When running without --fix, Ruff's formatter hook can be placed before or after Ruff's lint hook.

https://github.com/astral-sh/ruff-pre-commit/

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@AA-Turner oh, I didn't realize. It's so popular that I tend to forget there's people who don't use it. I think I first contributed to pre-commit in 2016…

But yeah, there's science to crafting a config for the pre-commit tool. And there are a few principles I found very useful in this context. Here's how I sort the entries:

  • fast checks first, followed by heavier linters (as in, flake8 goes before pylint, for example, by MyPy would probably be in between)
  • formatters before linters (as in, autopep8 and then flake8)
  • if there are groups of formatters+linters touching different file categories, the entire category can be moved together, no need to put autopep8 and docformatter together as they change different file types

pre-commit itself does not modify things by itself, so you can say it's idempotent in this sense. It's the checks that call other tools that modify files. I think if a check changes files (pre-commit docs sometimes call these “fixers”, not “formatters”), pre-commit will then show that entry as failed (although, it probably relies on the called program's return code). When something changed, it's useful to know what, so it's best to call the tool with --show-diff-on-failure. I think the https://pre-commit.ci web service does this by default (this service is also able to push a commit with the changed files back to pull requests, by the way). Some people only use this CLI flag in CI, while others stick it into the command runner like tox so it's active locally when the contributors run linting.

Feel free to tag me for reviews in future PRs if you need any more insight on this topic.

name: Run Ruff (format) on Doc/
args: [--exit-non-zero-on-fix]
args: [--check]
files: ^Doc/

- repo: https://github.com/psf/black-pre-commit-mirror
Expand Down