Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

confine pre-commit to stages #2213

Merged
merged 1 commit into from
Dec 13, 2023

Conversation

davidculley
Copy link
Contributor

See https://pre-commit.com/#confining-hooks-to-run-at-certain-stages

If you are authoring a tool, it is usually a good idea to provide an appropriate stages property. For example a reasonable setting for a linter or code formatter would be stages: [pre-commit, pre-merge-commit, pre-push, manual].

See https://pre-commit.com/#confining-hooks-to-run-at-certain-stages

> If you are authoring a tool, it is usually a good idea to provide an appropriate `stages` property. For example a reasonable setting for a linter or code formatter would be `stages: [pre-commit, pre-merge-commit, pre-push, manual]`.
@staticdev
Copy link
Collaborator

@davidculley thanks for contributing. Could you elaborate on the changes it will bring to current behavior of pre-commit and what are pros and cons of having this? What are the default stages?

@davidculley
Copy link
Contributor Author

Sure.

pre-commit supports many types of git hooks. See:

https://pre-commit.com/#supported-git-hooks

Of all these possible types, only the type pre-commit is used/installed by default. That is, after pip install pre-commit && pre-commit install, you only have the type pre-commit installed. Optionally, you can install additional types.

For example, you could additionally install the hook type commit-msg to make pre-commit ensure that commit messages follow the convention conventional commits, or to prevent duplicate Signed-off-by trailers. You'd install that optional additional hook type with the command pre-commit install --hook-type commit-msg.

Without the changes in this pull request, pre-commit would run isort at every stage that is installed. But not every stage (hook type) makes sense for running isort. isort should run only at a sensible subset of all the available hook types.

With the changes in this pull request, isort would no longer run, for example, after amending a commit or after a rebase, even if a user had installed the hook type post-rewrite using pre-commit install --hook-type post-rewrite because, according to the pre-commit documentation, it doesn't make sense to run isort at that stage.

Providers of hooks can select which git hooks they run on by setting the stages property in .pre-commit-hooks.yaml.

a reasonable setting for a linter or code formatter would be stages: [pre-commit, pre-merge-commit, pre-push, manual].

If stages is not set […] the default value will be pulled from the top-level default_stages option (which defaults to all stages). By default, tools are enabled for every hook type that pre-commit supports.

I don't know if there are cons to not having this, but the developer of pre-commit recommends adding it:

If you are authoring a tool, it is usually a good idea to provide an appropriate stages property.

One point to remember is:

In mid/late 2024, we'd have to require at least pre-commit version 3.2.0 and change

minimum_pre_commit_version: '2.9.2'

to

minimum_pre_commit_version: '3.2.0'

Reason: Probably around that time, pre-commit will be released in version 4.x.x and version 4 will require

stages: [pre-commit, pre-merge-commit, pre-push, manual]

(notice the prefix pre-) instead of the current proposal

stages: [commit, merge-commit, push, manual]

but the new version 4 syntax will not work with versions of pre-commit older than 3.2.0, so some users still stuck on a version older than 3.2.0 would complain if we added the newer syntax right away today.

@staticdev
Copy link
Collaborator

@davidculley thanks a lot for the explanation! It is clear now, lgtm.

Copy link
Collaborator

@staticdev staticdev left a comment

Choose a reason for hiding this comment

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

Thanks again.

@staticdev staticdev merged commit e38702f into PyCQA:main Dec 13, 2023
18 checks passed
@davidculley
Copy link
Contributor Author

You're welcome. I'm happy I could be useful.

@davidculley davidculley deleted the confine-precommit-to-stages branch December 14, 2023 14:35
@staticdev
Copy link
Collaborator

Btw @davidculley your change is already in last release

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants