feat(install): allow to install hook locally with husky #1150
Conversation
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #1150 +/- ##
==========================================
- Coverage 91.97% 91.94% -0.03%
==========================================
Files 144 144
Lines 6202 6221 +19
==========================================
+ Hits 5704 5720 +16
- Misses 498 501 +3
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
GG-HH
changed the title
ggshield/cmd/install.py
Outdated
| return hooks_path | ||
|
|
||
|
|
||
| def get_local_hooks_path() -> Optional[Path]: |
There was a problem hiding this comment.
Having both get_local_hook_dir_path() and get_local_hooks_path() is confusing. Maybe name this one get_hooks_path_from_local_git_config() (a bit long, but unambiguous)?
There was a problem hiding this comment.
I renamed to get_git_local_hooks_path, wdyt ?
There was a problem hiding this comment.
It does not look like you renamed it 😅. I would prefer my proposal but I can live with yours.
tests/functional/test_install.py
Outdated
| from tests.repository import Repository | ||
|
|
||
|
|
||
| def test_install_local_detects_husky(tmp_path: Path) -> None: |
There was a problem hiding this comment.
I would expect a functional test to use Husky for real. Is it doable?
I think if you replace the use of run_ggshield() with a call to install_local() then this code can become a unit test for install_local().
There was a problem hiding this comment.
A functional test with husky feels a bit heavy as it would introduce the need for npm and husky. As you suggested, I moved to a unit test
There was a problem hiding this comment.
Yes, that could be heavy indeed, though functional tests are allowed to be heavy. I am OK with turning it into a unit test, but... it does not look like you did it.
Change function name to get hook Move functional test to unit test
GG-HH
left a comment
GG-HH
left a comment
There was a problem hiding this comment.
Thanks for the first review!
ggshield/cmd/install.py
Outdated
| return hooks_path | ||
|
|
||
|
|
||
| def get_local_hooks_path() -> Optional[Path]: |
There was a problem hiding this comment.
I renamed to get_git_local_hooks_path, wdyt ?
tests/functional/test_install.py
Outdated
| from tests.repository import Repository | ||
|
|
||
|
|
||
| def test_install_local_detects_husky(tmp_path: Path) -> None: |
There was a problem hiding this comment.
A functional test with husky feels a bit heavy as it would introduce the need for npm and husky. As you suggested, I moved to a unit test
|
Hello @agateau-gg have you had the opportunity to proof read again my code ? |
ggshield/cmd/install.py
Outdated
| return hooks_path | ||
|
|
||
|
|
||
| def get_local_hooks_path() -> Optional[Path]: |
There was a problem hiding this comment.
It does not look like you renamed it 😅. I would prefer my proposal but I can live with yours.
tests/functional/test_install.py
Outdated
| from tests.repository import Repository | ||
|
|
||
|
|
||
| def test_install_local_detects_husky(tmp_path: Path) -> None: |
There was a problem hiding this comment.
Yes, that could be heavy indeed, though functional tests are allowed to be heavy. I am OK with turning it into a unit test, but... it does not look like you did it.
GG-HH
force-pushed
the
gg-hh/1143/husky_install
branch
from
4cfbc12 to
0e73973
Compare
Fixes #1143
Context
Trying to fix error #1143
What has been done
In case the git hooks path is the husky path, add the ggshield command in the husky configuration file
Validation
See issue
PR check list
skip-changeloglabel has been added to the PR.