Skip to content

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