Add CI detection to skip installing hooks#142
Conversation
|
📝 WalkthroughWalkthroughThe Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
simple-git-hooks.js (1)
465-468: Add tests for the new CI branch inskipInstall().Current tests cover
SKIP_INSTALL_SIMPLE_GIT_HOOKS, but not CI detection. Please add at least one case asserting CI causesskipInstall()to returntrue(and skip hook setup in postinstall/CLI flows).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@simple-git-hooks.js` around lines 465 - 468, Add a unit test for the CI branch in skipInstall(): mock/stub the is-ci module to return true (or set process.env.CI) and assert that skipInstall() returns true; additionally add an integration-style test that simulates the postinstall/CLI flow (calling the function that triggers hook setup) and assert it does not attempt to install hooks when is-ci is true. Target the skipInstall() function and the postinstall/CLI entrypoint used in the existing tests, restoring the original is-ci behavior after the test.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@simple-git-hooks.js`:
- Around line 465-468: Add a unit test for the CI branch in skipInstall():
mock/stub the is-ci module to return true (or set process.env.CI) and assert
that skipInstall() returns true; additionally add an integration-style test that
simulates the postinstall/CLI flow (calling the function that triggers hook
setup) and assert it does not attempt to install hooks when is-ci is true.
Target the skipInstall() function and the postinstall/CLI entrypoint used in the
existing tests, restoring the original is-ci behavior after the test.
| console.log(`[INFO] SKIP_INSTALL_SIMPLE_GIT_HOOKS is set to "${SKIP_INSTALL_SIMPLE_GIT_HOOKS}", skipping installing hook.`) | ||
| return true; | ||
| } | ||
| if (require('is-ci')) { |
There was a problem hiding this comment.
This is a breaking change, not everyone wants this. And you can easily set SKIP_INSTALL_SIMPLE_GIT_HOOKS=1 env on CI.
https://github.com/toplenboren/simple-git-hooks?tab=readme-ov-file#i-want-to-skip-git-hooks
There was a problem hiding this comment.
agree, then it could be shipped as v3 🙂
It's necessary to add
is-cito yarn lock .cc @toplenborenSummary by CodeRabbit