Report hook installation fail #578

Bogay · 2022-09-07T21:33:38Z

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 · 2022-09-07T21:38:57Z

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 · 2022-09-10T09:51:10Z

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

Yep that would be great

commitizen/commands/init.py

tests/test_cli.py

Bogay · 2022-09-28T16:40:09Z

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

Lee-W

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!

Lee-W · 2022-10-17T09:43:09Z

commitizen/commands/init.py

+            ],
+        ).ask()
+        if hook_types:
+            if not self._install_pre_commit_hook(hook_types):


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

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:

Yep, that sounds great!

Lee-W · 2022-10-17T09:43:38Z

commitizen/commands/init.py

        return tag_format

-    def _install_pre_commit_hook(self):
+    def _search_pre_commit(self):


nitpick: missing return type anntation

Lee-W · 2022-10-17T09:44:41Z

commitizen/commands/init.py

+        )
+        c = cmd.run(cmd_str)
+        if c.return_code != 0:
+            out.error(f"Error running {cmd_str}. Outputs are attached below:")


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

Bogay · 2022-10-18T13:00:25Z

commitizen/commands/init.py

+        cmd_str = "pre-commit install " + "".join(
+            f"--hook-type {ty}" for ty in hook_types
+        )


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

)

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 · 2022-10-20T18:58:48Z

@Lee-W it's done 👍

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

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

instead of returning a `False` value.

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

Lee-W

@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 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 added pr-status: wait-for-review waiting-for-update and removed pr-status: wait-for-review waiting-for-update labels Sep 19, 2022

Bogay force-pushed the fix/report-hook-installation-fail branch from c81ad48 to ca08c91 Compare September 28, 2022 16:22

Bogay requested a review from Lee-W September 28, 2022 16:40

Lee-W added pr-status: wait-for-review and removed in progress labels Oct 3, 2022

Lee-W reviewed Oct 17, 2022

View reviewed changes

Lee-W added issue-status: wait-for-response and removed pr-status: wait-for-review labels Oct 17, 2022

Bogay commented Oct 18, 2022

View reviewed changes

Bogay requested a review from Lee-W October 20, 2022 18:56

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

fix(init): report error when hook installation failed

c3d34bc

style(test): add type hints for mocker

3bf0190

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

test: fix some incompatible types of function calls

5be0769

test(init): pre-commit failed cases

f1b581c

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

refactor(init): _install_pre_commit_hook raise error when failed

599c10f

instead of returning a `False` value.

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 force-pushed the fix/report-hook-installation-fail branch from df02f29 to a23ef07 Compare December 29, 2022 10:01

Lee-W approved these changes Dec 29, 2022

View reviewed changes

Lee-W added wait-for-merge and removed pr-status: wait-for-review labels Dec 29, 2022

Lee-W merged commit a1e0383 into commitizen-tools:master Dec 31, 2022

Bogay deleted the fix/report-hook-installation-fail branch April 29, 2023 13:11

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Uh oh!

Report hook installation fail #578

Report hook installation fail #578

Uh oh!

Bogay commented Sep 7, 2022 •

edited

Loading

Uh oh!

codecov bot commented Sep 7, 2022 •

edited

Loading

Uh oh!

Lee-W commented Sep 10, 2022

Uh oh!

Uh oh!

Uh oh!

Bogay commented Sep 28, 2022

Uh oh!

Lee-W left a comment

Uh oh!

Lee-W Oct 17, 2022

Uh oh!

Bogay Oct 18, 2022

Uh oh!

Lee-W Oct 20, 2022

Uh oh!

Lee-W Oct 17, 2022

Uh oh!

Lee-W Oct 17, 2022

Uh oh!

Bogay Oct 18, 2022

Uh oh!

Lee-W Oct 20, 2022

Uh oh!

Bogay commented Oct 20, 2022

Uh oh!

Lee-W left a comment

Uh oh!

Uh oh!

Uh oh!

Report hook installation fail #578

Report hook installation fail #578

Uh oh!

Conversation

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 left a comment

Choose a reason for hiding this comment

Uh oh!

Lee-W Oct 17, 2022

Choose a reason for hiding this comment

Uh oh!

Bogay Oct 18, 2022

Choose a reason for hiding this comment

Uh oh!

Lee-W Oct 20, 2022

Choose a reason for hiding this comment

Uh oh!

Lee-W Oct 17, 2022

Choose a reason for hiding this comment

Uh oh!

Lee-W Oct 17, 2022

Choose a reason for hiding this comment

Uh oh!

Bogay Oct 18, 2022

Choose a reason for hiding this comment

Uh oh!

Lee-W Oct 20, 2022

Choose a reason for hiding this comment

Uh oh!

Bogay commented Oct 20, 2022

Uh oh!

Lee-W left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Bogay commented Sep 7, 2022 •

edited

Loading

codecov bot commented Sep 7, 2022 •

edited

Loading