Skip to content

feat(pre-commit): Add commitizen-branch hook #517

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

Merged
merged 2 commits into from
Aug 21, 2022

Conversation

Kurt-von-Laven
Copy link
Contributor

@Kurt-von-Laven Kurt-von-Laven commented May 22, 2022

Description

Check all commit messages on the current branch. This is useful for checking commit messages after the fact (e.g., pre-push or in CI) since the existing hook only works at commit time. Expand the documentation of the pre-existing commitizen hook to clarify the relationship between them. I wasn't sure whether it would be appropriate to add a section to this documentation.

Remove no longer needed commit-msg stage from self-use of commitizen pre-commit hook in .pre-commit-config.yaml. In v2.27.1, the commitizen pre-commit hook began specifying that it runs on the commit-msg stage in the hook definition in .pre-commit-hooks.yaml, so it is no longer necessary to specify the hook stage when using the hook in .pre-commit-config.yaml.

Checklist

  • Add test cases to all the changes you introduce
  • Run ./scripts/format and ./scripts/test locally to ensure this change passes linter check and test
  • Test the changes on the local machine manually
  • Update the documentation for the changes

Expected behavior

The commitizen hook continues to work as before, allowing empty commit messages. The commitizen-branch hook checks all commit messages on your branch, and doesn't allow empty commit messages.

Steps to Test This Pull Request

  1. Replace your Commitizen hook config in your .pre-commit-config.yaml with the following:

    repos:
      - repo: https://github.com/Kurt-von-Laven/commitizen
        rev: pre-commit-hook
        hooks:
          - id: commitizen
          - id: commitizen-branch
            stages: [push]
  2. Ensure that you have pre-commit 1.4.3 or higher installed.

  3. Install commit-msg and pre-push hooks if you haven't already via: pre-commit install --hook-type commit-msg pre-push.

  4. Commit some changes to your repository.

  5. If the commit message was invalid, the commitizen hook will fail, and otherwise it will succeed.

  6. Commit a change with an empty commit message: git commit --allow-empty-message -m "".

  7. Attempt to push your branch.

  8. The push fails with an error regarding the empty commit message.

Additional context

Follows on #512 and #514.

@Kurt-von-Laven Kurt-von-Laven changed the title Add commitizen-branch pre-commit hook ci(pre-commit): Add commitizen-branch pre-commit hook May 22, 2022
@Kurt-von-Laven Kurt-von-Laven changed the title ci(pre-commit): Add commitizen-branch pre-commit hook ci(pre-commit): Add commitizen-branch hook May 22, 2022
@codecov
Copy link

codecov bot commented May 22, 2022

Codecov Report

Merging #517 (01ce73b) into master (764861f) will decrease coverage by 0.32%.
The diff coverage is n/a.

❗ Current head 01ce73b differs from pull request most recent head bd4aa6f. Consider uploading reports for the commit bd4aa6f to get more accurate results

@@            Coverage Diff             @@
##           master     #517      +/-   ##
==========================================
- Coverage   98.25%   97.93%   -0.33%     
==========================================
  Files          39       39              
  Lines        1551     1551              
==========================================
- Hits         1524     1519       -5     
- Misses         27       32       +5     
Flag Coverage Δ
unittests 97.93% <ø> (-0.33%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
commitizen/changelog.py 96.72% <0.00%> (-2.72%) ⬇️
commitizen/cli.py 93.93% <0.00%> (ø)
commitizen/cmd.py 100.00% <0.00%> (ø)
commitizen/git.py 100.00% <0.00%> (ø)
commitizen/commands/bump.py 96.42% <0.00%> (ø)
commitizen/commands/check.py 100.00% <0.00%> (ø)
...en/cz/conventional_commits/conventional_commits.py 100.00% <0.00%> (ø)

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

@Kurt-von-Laven Kurt-von-Laven changed the title ci(pre-commit): Add commitizen-branch hook feat(pre-commit): Add commitizen-branch hook May 22, 2022
Copy link
Member

@Lee-W Lee-W left a comment

Choose a reason for hiding this comment

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

@woile Overall I'm good with this feature. One of the things come to my mind is whether we really need 2 cz check hooks? It seems users can config such behavior on their .pre-commit-config.yaml as well

@Lee-W Lee-W added discussion type: feature A new enhacement proposal labels Aug 14, 2022
In v2.27.1, the commitizen pre-commit hook began specifying that it runs
on the commit-msg stage in .pre-commit-hooks.yaml, so it is no longer
necessary to specify the hook stage when using the hook in
.pre-commit-config.yaml.
Check all commit messages on the current branch. This is useful for
checking commit messages after the fact (e.g., pre-push or in CI) since
the existing hook only works at commit time. Expand the documentation of
the pre-existing commitizen hook to clarify the relationship between
them.
@Kurt-von-Laven
Copy link
Contributor Author

Users can certainly write their own pre-commit hooks, but I consider them tricky to write and test since many implementations I see that haven't already been reviewed by the author of pre-commit get details wrong. I also believe many users will not be aware that the commit-msg hook relies on the developer to have installed the pre-commit hooks correctly since it can't be run in CI. It is a good hook in that it offers real-time feedback, but it is also trivially circumvented (most likely by accident). Tools like pre-commit.ci, pre-commit/pre-commit-action, and ScribeMD/pre-commit-action make it trivial to run the proposed pre-push hook in CI, which is not possible with the existing commit-msg hook.

@Kurt-von-Laven
Copy link
Contributor Author

The test failures seem unrelated to this pull request.

Copy link
Member

@Lee-W Lee-W left a comment

Choose a reason for hiding this comment

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

@woile I'm good with this one. Could you please take a look? Thanks! As for the failed tests, I'll send a separate PR to fix it.

@woile woile merged commit 0dc39a2 into commitizen-tools:master Aug 21, 2022
@Kurt-von-Laven Kurt-von-Laven deleted the pre-commit-hook branch August 21, 2022 21:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type: feature A new enhacement proposal
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants