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

chore: Improve pre-commit configuration #415

Merged

Conversation

JP-Ellis
Copy link
Contributor

This PR has two logical changes which should hopefully improve the contributor experience. It will ensure that pre-commit correctly runs, and is correctly installed for everyone.

The two commits are:

  • chore: Expand default hooks installed (d08cd1b)

    When developers run pre-commit install, pre-commit by default only installed the pre-commit hook. The configuration also makes use of the commit-msg hook which would remain unused. The developer is required to run pre-commit install -t commit-msg (in addition to the default install), which they would only know from inspecting the pre-commit configuration file.

    By specifying both hooks in default_install_hook_types, a simple pre-commit install will ensure that both hooks are installed. This should hopefully provide a better developer experience and catch a few lints before they are pushed to a branch and cause CI to fail.

    Signed-off-by: JP-Ellis [email protected]

  • chore: Replace invalid git hook stage (2bd88eb)

    While working on my other PRs targetting this repository, I was wondering why CI failed despite having installed pre-commit hooks. I quickly realized that the stages: are being overwritten from their default and use the non-existent commit stage. See for example the git documentation which list the following commit-related hooks:

    • pre-commit
    • pre-merge-commit
    • prepare-commit-msg
    • commit-msg
    • post-commit

    This should hopefully improve the experience of any possible future contributors and avoid unnecessary back-and-forth due to CI failures.

    Signed-off-by: JP-Ellis [email protected]

While working on my other PRs targetting this repository, I was
wondering why CI failed despite having installed pre-commit hooks. I
quickly realized that the `stages:` are being overwritten from their
default and use the non-existent `commit` stage. See for example the git
documentation which list the following commit-related hooks:

- pre-commit
- pre-merge-commit
- prepare-commit-msg
- commit-msg
- post-commit

This should hopefully improve the experience of any possible future
contributors and avoid unnecessary back-and-forth due to CI failures.

Signed-off-by: JP-Ellis <[email protected]>
When developers run `pre-commit install`, pre-commit by default only
installed the `pre-commit` hook. The configuration also makes use of
the `commit-msg` hook which would remain unused. The developer is
required to run `pre-commit install -t commit-msg` (in addition to the
default install), which they would only know from inspecting the
pre-commit configuration file.

By specifying both hooks in `default_install_hook_types`, a simple
`pre-commit install` will ensure that _both_ hooks are installed. This
should hopefully provide a better developer experience and catch a few
lints _before_ they are pushed to a branch and cause CI to fail.

Signed-off-by: JP-Ellis <[email protected]>
@coveralls
Copy link

Pull Request Test Coverage Report for Build 12387491914

Warning: This coverage report may be inaccurate.

This pull request's base commit is no longer the HEAD commit of its target branch. This means it includes changes from outside the original pull request, including, potentially, unrelated coverage changes.

Details

  • 0 of 0 changed or added relevant lines in 0 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage remained the same at 5.0%

Totals Coverage Status
Change from base Build 12125583972: 0.0%
Covered Lines: 24
Relevant Lines: 480

💛 - Coveralls

@epage epage merged commit ed39729 into crate-ci:master Dec 18, 2024
17 checks passed
@JP-Ellis JP-Ellis deleted the chore/auto-install-commit-msg-pre-commit branch December 18, 2024 17:04
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.

3 participants