-
-
Notifications
You must be signed in to change notification settings - Fork 299
Precommit docs #210
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
Precommit docs #210
Conversation
Codecov Report
@@ Coverage Diff @@ ## master #210 +/- ## ======================================= Coverage 95.64% 95.64% ======================================= Files 34 34 Lines 942 942 ======================================= Hits 901 901 Misses 41 41
Continue to review full report at Codecov.
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for your contribution! I've added some comments.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Adding this config file is not enough for pre-commit, I'd suggest add the pre-commit install --hook-type commit-msg command
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm. This already assumes that the dev has run pre-commit before tho. When I was trying this out for myself I just added that snippet to my config and reran pre-commit. I think the stages: [commit-msg] is enough.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
By default commit-msg is not set for pre-commit. Simply adding these lines to your .pre-commit-config.yaml will not trigger pre-commit at commit-msg stage.
The following is how I test it.
e.g.,
mkdir test-project cd test-project git init # add a .pre-commit-config.yaml without commitizen git add . git commit pre-commit install # add commitizen config to .pre-commit-config.yaml git add . git commit
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hey @Lee-W, just to follow up, you're right. It needs the pre-commit install --hook-type commit-msg command.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah, I miss this part when I merged this PR. No worries. I'll add it in another PR.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the update 🙂 We have one more discussion to resolve and then we can merge it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
By default commit-msg is not set for pre-commit. Simply adding these lines to your .pre-commit-config.yaml will not trigger pre-commit at commit-msg stage.
The following is how I test it.
e.g.,
mkdir test-project cd test-project git init # add a .pre-commit-config.yaml without commitizen git add . git commit pre-commit install # add commitizen config to .pre-commit-config.yaml git add . git commit
Can't we add to pyproject.toml
version_files = [
"pyproject.toml:version",
"commitizen/__version__.py",
"docs/README.md:\trev:"
]
or something like that?
Can't we add to pyproject.toml
version_files = [ "pyproject.toml:version", "commitizen/__version__.py", "docs/README.md:\trev:" ]or something like that?
Sure. But I think it'll just make it more verbose than it has to be. Other python libs that use pre-commit—pre-commit included just use master or stable.
master is fine by me as well, I have no issues with it
Types of changes
Please put an
xin the box that appliesDescription
Add instructions for pre-commit integration in the README.md itself.
Checklist:
./script/lintand./script/testlocally to ensure this change passes linter check and testRelated Issue
#196 #209