Report hook installation fail #578

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

Jump to bottom

Merged

Lee-W merged 7 commits into commitizen-tools:master from Bogay:fix/report-hook-installation-fail

Dec 31, 2022

Merged

Report hook installation fail #578

Lee-W merged 7 commits into commitizen-tools:master from Bogay:fix/report-hook-installation-fail

Dec 31, 2022

Conversation

Bogay

Copy link

Contributor

@Bogay Bogay commented Sep 7, 2022 •

edited

Loading

Description

This PR make cz report errors if user want to install pre-commit hook and failed, which fixs #428.

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

Run cz init will print error if there is something wrong.

❯ cz init
? Please choose a supported config file: (default: pyproject.toml) pyproject.toml
? Please choose a cz (commit rule): (default: cz_conventional_commits) cz_conventional_commits
No Existing Tag. Set tag to v0.0.1
? Please enter the correct version format: (default: "$version")
? Do you want to install pre-commit hook? Yes
pre-commit is not installed in current environement.
Installation failed. See error outputs for more information.

And the exit code should not be 0.

❯ echo $?
24

Steps to Test This Pull Request

# Run `cz init` with default choices
cz init
# Check the exit code should not be 0
test $? != 0

Additional context

I found that TestPreCommitCases has some duplicated code. Should they be extracted to methods?

@codecov

Copy link

codecov bot commented Sep 7, 2022 •

edited

Loading

Codecov Report

Base: 97.92% // Head: 98.03% // Increases project coverage by +0.10% 🎉

Coverage data is based on head (a23ef07) compared to base (db42a95).
Patch coverage: 85.96% of modified lines in pull request are covered.

Additional details and impacted files

@@ Coverage Diff @@
## master #578 +/- ##
==========================================
+ Coverage 97.92% 98.03% +0.10% 
==========================================
 Files 35 39 +4 
 Lines 1252 1676 +424 
==========================================
+ Hits 1226 1643 +417 
- Misses 26 33 +7

Flag	Coverage Δ
unittests	`98.03% <85.96%> (+0.10%)`	⬆️

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

Impacted Files	Coverage Δ
commitizen/commands/init.py	`87.93% <84.00%> (-3.74%)`	⬇️
commitizen/__version__.py	`100.00% <100.00%> (ø)`
commitizen/commands/changelog.py	`98.86% <100.00%> (ø)`
commitizen/commands/check.py	`100.00% <100.00%> (ø)`
commitizen/exceptions.py	`100.00% <100.00%> (ø)`
commitizen/bump.py	`100.00% <0.00%> (ø)`
commitizen/__init__.py	`100.00% <0.00%> (ø)`
commitizen/changelog.py	`99.43% <0.00%> (ø)`

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

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

@Lee-W

Copy link

Member

Lee-W commented Sep 10, 2022

I found that TestPreCommitCases has some duplicated code. Should they be extracted to methods?

Yep that would be great

Lee-W

Lee-W reviewed

Sep 10, 2022

View reviewed changes

commitizen/commands/init.py Outdated Show resolved Hide resolved

tests/test_cli.py Show resolved Hide resolved

@Lee-W Lee-W added pr-status: wait-for-review waiting-for-update and removed pr-status: wait-for-review waiting-for-update labels

Sep 19, 2022

@Bogay Bogay force-pushed the fix/report-hook-installation-fail branch from c81ad48 to ca08c91 Compare

September 28, 2022 16:22

@Bogay

Copy link

Contributor Author

Bogay commented Sep 28, 2022

@Lee-W I think it's ready to be reviewed.

@Bogay Bogay requested a review from Lee-W

September 28, 2022 16:40

@Lee-W Lee-W added pr-status: wait-for-review and removed in progress labels

Oct 3, 2022

Lee-W

Lee-W reviewed

Oct 17, 2022

View reviewed changes

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.

Hi @Bogay sorry for the late review. Just one minor design issue I'd like to discuss. Others are just minor nitpick. You could fix it if you have time. Otherwise we could do that in the future. We're almost ready for this merge!

commitizen/commands/init.py Outdated

).ask()

if hook_types:

if not self._install_pre_commit_hook(hook_types):

Copy link

Member

@Lee-W Lee-W Oct 17, 2022

Choose a reason for hiding this comment

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

I think we should put the following code into _exec_install_pre_commit_hook. This seems to be the responsibility of that function. @Bogay what do you think?

 raise InitFailedError(
 "Installation failed. See error outputs for more information."
 )

Copy link

Contributor Author

@Bogay Bogay Oct 18, 2022

Choose a reason for hiding this comment

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

Yeah, I think that's reasonable. We can let _install_pre_commit_hook raise an exception instead of return a bool.
Maybe we can just put all error message inside the InitFailedError so that we don't need extra out.error calls?

This is what the error output looks like:
圖片

Copy link

Member

@Lee-W Lee-W Oct 20, 2022

Choose a reason for hiding this comment

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

Yep, that sounds great!

commitizen/commands/init.py Outdated

return tag_format

def _install_pre_commit_hook(self):

def _search_pre_commit(self):

Copy link

Member

@Lee-W Lee-W Oct 17, 2022

Choose a reason for hiding this comment

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

nitpick: missing return type anntation

commitizen/commands/init.py Outdated

)

c = cmd.run(cmd_str)

if c.return_code != 0:

out.error(f"Error running {cmd_str}. Outputs are attached below:")

Copy link

Member

@Lee-W Lee-W Oct 17, 2022

Choose a reason for hiding this comment

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

nitpick: I might suggest invoke one out.error and concatenation the string instead.

@Lee-W Lee-W added issue-status: wait-for-response and removed pr-status: wait-for-review labels

Oct 17, 2022

Bogay

Bogay commented

Oct 18, 2022

View reviewed changes

commitizen/commands/init.py Outdated

Comment on lines 129 to 145

cmd_str = "pre-commit install " + "".join(

f"--hook-type {ty}" for ty in hook_types

)

Copy link

Contributor Author

@Bogay Bogay Oct 18, 2022

Choose a reason for hiding this comment

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

I found that I forgot to add a space between --hook-type options...
@Lee-W Do you have any idea how to write tests for this?
Because it depends on pre-commit's behavior, it's harder to test.

Suggested change

cmd_str = "pre-commit install " + "".join(

f"--hook-type {ty}" for ty in hook_types

)

cmd_str = "pre-commit install " + "".join(

f"--hook-type {ty}" for ty in hook_types

)

Copy link

Member

@Lee-W Lee-W Oct 20, 2022

Choose a reason for hiding this comment

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

we could verify whether the code generate the pre-commit command we want. e.g., mock the caller function of cmd_str to check

@Bogay Bogay requested a review from Lee-W

October 20, 2022 18:56

@Bogay

Copy link

Contributor Author

Bogay commented Oct 20, 2022

@Lee-W it's done 👍

@Lee-W Lee-W added pr-status: wait-for-review and removed issue-status: wait-for-response labels

Oct 23, 2022

Bogay added 7 commits

December 29, 2022 17:53

@Bogay @Lee-W


 fix(init): report error when hook installation failed

c3d34bc

@Bogay @Lee-W


 style(test): add type hints for mocker

3bf0190

See more: https://pytest-mock.readthedocs.io/en/latest/remarks.html#type-annotations

@Bogay @Lee-W


 test: fix some incompatible types of function calls

5be0769

@Bogay @Lee-W


 test(init): pre-commit failed cases

f1b581c

@Bogay @Lee-W


 feat(init): allow user to select which type of pre commit hooks to in...

86c4f60

...stall
See more: https://commitizen-tools.github.io/commitizen/#integrating-with-pre-commit

@Bogay @Lee-W


 refactor(init): _install_pre_commit_hook raise error when failed

599c10f

instead of returning a `False` value.

@Bogay @Lee-W


 fix(init): space between --hook-type options

a23ef07

there should be a space between `--hook-type` options in generated
`pre-commit` command string.

@Lee-W Lee-W force-pushed the fix/report-hook-installation-fail branch from df02f29 to a23ef07 Compare

December 29, 2022 10:01

Lee-W

Lee-W approved these changes

Dec 29, 2022

View reviewed changes

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.

@Bogay Sorry for taking so long. I think we're ready to merge this one!

@woile I plan to merge this later this week. Let me know if you want to take a deeper look.

@Lee-W Lee-W added wait-for-merge and removed pr-status: wait-for-review labels

Dec 29, 2022

@Lee-W Lee-W merged commit a1e0383 into commitizen-tools:master

Dec 31, 2022

@Bogay Bogay deleted the fix/report-hook-installation-fail branch

April 29, 2023 13:11

Labels

None yet

2 participants

@Bogay @Lee-W

Uh oh!

Report hook installation fail #578

Report hook installation fail #578

Uh oh!

Conversation

@Bogay Bogay commented Sep 7, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Checklist

Expected behavior

Steps to Test This Pull Request

Additional context

Uh oh!

codecov bot commented Sep 7, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Lee-W commented Sep 10, 2022

Uh oh!

Uh oh!

Uh oh!

Bogay commented Sep 28, 2022

Uh oh!

@Lee-W Lee-W left a comment

Choose a reason for hiding this comment

Uh oh!

@Lee-W Lee-W Oct 17, 2022

Choose a reason for hiding this comment

Uh oh!

@Bogay Bogay Oct 18, 2022

Choose a reason for hiding this comment

Uh oh!

@Lee-W Lee-W Oct 20, 2022

Choose a reason for hiding this comment

Uh oh!

@Lee-W Lee-W Oct 17, 2022

Choose a reason for hiding this comment

Uh oh!

@Lee-W Lee-W Oct 17, 2022

Choose a reason for hiding this comment

Uh oh!

@Bogay Bogay Oct 18, 2022

Choose a reason for hiding this comment

Uh oh!

@Lee-W Lee-W Oct 20, 2022

Choose a reason for hiding this comment

Uh oh!

Bogay commented Oct 20, 2022

Uh oh!

@Lee-W Lee-W left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

@Bogay Bogay commented Sep 7, 2022 •

edited

Loading

codecov bot commented Sep 7, 2022 •

edited

Loading