Skip to content

feat(bump): add optional --no-verify argument for bump command #183

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

Conversation

kudlatyamroth
Copy link
Contributor

@kudlatyamroth kudlatyamroth commented May 4, 2020

fix: #164

When we have in repo some pre-commit hooks, like black, mypy or anything like that
cz bump fails, even if pre-commit hooks passed.

it is strange as pre-commit write messages to sys.stdout.buffer not stderr...

example pre-commit config:

repos:
  - repo: local
    hooks:
      - id: flake8
        name: flake8
        stages: [commit]
        language: system
        entry: poetry run flake8
        types: [python]
        exclude: setup.py

      - id: mypy
        name: mypy
        stages: [commit]
        language: system
        entry: poetry run mypy
        types: [python]
        pass_filenames: false

This PR add optional --no-verify argument to bump command

@codecov
Copy link

codecov bot commented May 4, 2020

Codecov Report

Merging #183 into master will increase coverage by 0.26%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #183      +/-   ##
==========================================
+ Coverage   91.76%   92.02%   +0.26%     
==========================================
  Files          35       35              
  Lines         947      953       +6     
==========================================
+ Hits          869      877       +8     
+ Misses         78       76       -2     
Flag Coverage Δ
#unittests 92.02% <100.00%> (+0.26%) ⬆️
Impacted Files Coverage Δ
commitizen/cli.py 91.17% <ø> (ø)
commitizen/__version__.py 100.00% <100.00%> (ø)
commitizen/commands/bump.py 88.37% <100.00%> (+3.37%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 86f67fd...638b38a. Read the comment docs.

@kudlatyamroth
Copy link
Contributor Author

@woile what do you think about that?
personally i think its hacky way to fix this, but can't find out why stdout from pre-commit is taken as stderr...

@Lee-W
Copy link
Member

Lee-W commented May 5, 2020

I also encounter the same problem. I'm thinking of adding a --no-verify argument to cz bump command to make it optional for users.

@kudlatyamroth
Copy link
Contributor Author

@Lee-W i changed this pr to make --no-verify as optional argument :)

@kudlatyamroth kudlatyamroth force-pushed the bugfix/fix-bumping-with-pre-commit branch from 846a31f to 4b79ab1 Compare May 5, 2020 05:47
@kudlatyamroth kudlatyamroth changed the title fix(bump/pre-commit): pass --no-verify to bump commit feat(bump): add optional --no-verify argument for bump command May 5, 2020
@kudlatyamroth kudlatyamroth force-pushed the bugfix/fix-bumping-with-pre-commit branch from 4b79ab1 to 6816c6e Compare May 5, 2020 05:50
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.

Thanks @kudlatyamroth ! This problem bothers me for some time. This PR looks great! But you might need to add some test cases to cover this new argument so that we'll be able to merge it.

"name": ["--no-verify"],
"action": "store_true",
"default": False,
"help": "disable git hooks during bumping",
Copy link
Member

Choose a reason for hiding this comment

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

I just take a look of the documentation of git. The help message is

--no-verify
    This option bypasses the pre-commit and commit-msg hooks.

I think it might be better if we can align with them

@kudlatyamroth kudlatyamroth force-pushed the bugfix/fix-bumping-with-pre-commit branch from 6816c6e to 5fc1033 Compare May 5, 2020 12:33
@kudlatyamroth kudlatyamroth force-pushed the bugfix/fix-bumping-with-pre-commit branch from 5fc1033 to b8f2894 Compare May 5, 2020 12:35
@kudlatyamroth
Copy link
Contributor Author

@Lee-W @woile
description changed, and test added :)
also updated with master :)

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.

LGTM! @kudlatyamroth Thanks for your help! Let's merge it!

@Lee-W Lee-W merged commit 7ae1850 into commitizen-tools:master May 5, 2020
@kudlatyamroth
Copy link
Contributor Author

@Lee-W thanks for merge :)
but there is no release. propably this line:
https://github.com/commitizen-tools/commitizen/actions/runs/96287380/workflow#L10

should check for bump: or bump: version

@Lee-W
Copy link
Member

Lee-W commented May 5, 2020

@kudlatyamroth It's probably due to #157 I'll take a look tomorrow

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.

cz bump fails when using pre-commit to check commit message
2 participants