Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly
Sign up
Appearance settings

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
woile merged 2 commits into commitizen-tools:master from Kurt-von-Laven:pre-commit-hook
Aug 21, 2022

Conversation

Copy link
Contributor

@Kurt-von-Laven Kurt-von-Laven commented May 22, 2022 •
edited
Loading

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
Copy link

codecov bot commented May 22, 2022 •
edited
Loading

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.
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.

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 reacted with rocket emoji
@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
Reviewers

@Lee-W Lee-W Lee-W approved these changes

Assignees
No one assigned
Labels
type: feature A new enhacement proposal
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

AltStyle によって変換されたページ (->オリジナル) /