-
Notifications
You must be signed in to change notification settings - Fork 220
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
Cleanup hook template #234
Cleanup hook template #234
Conversation
cmd/templates/hook.tmpl
Outdated
elif test -f "$dir/node_modules/@arkweid/lefthook/bin/lefthook{{.Extension}}" | ||
elif test -f "$dir/node_modules/@arkweid/lefthook/bin/index.js" | ||
then | ||
eval "$dir/node_modules/@arkweid/lefthook/bin/lefthook{{.Extension}} $@" | ||
eval "$dir/node_modules/@arkweid/lefthook/bin/index.js $@" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This doesn't work. The test is always true
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You mean test -f "$dir/node_modules/@arkweid/lefthook/bin/index.js"
always return true
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes. There is no point in the test. Also, you can't eval JavaScript directly. In the new unreleased version, there is no need for index.js
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
test -f "$dir/node_modules/@arkweid/lefthook/bin/index.js"
only returns true if lefthook
is installed via npm/yarn
When uninstalled it would only echo Can't find lefthook in PATH
If lefthook
installed via Homebrew, the test -f
line won't even be called
My custom `pre-commit` file
#!/bin/sh
if [ "$LEFTHOOK" = "0" ]; then
exit 0
fi
if [ -t 1 ] ; then
exec < /dev/tty ; # <- enables interactive shell
fi
dir="$(git rev-parse --show-toplevel)"
call_lefthook()
{
if lefthook -h >/dev/null 2>&1
then
eval lefthook $@
elif test -f "$dir/node_modules/@arkweid/lefthook/bin/index.js"
then
eval "$dir/node_modules/@arkweid/lefthook/bin/index.js $@"
elif bundle exec lefthook -h >/dev/null 2>&1
then
bundle exec lefthook $@
elif npx lefthook -h >/dev/null 2>&1
then
npx lefthook $@
elif yarn lefthook -h >/dev/null 2>&1
then
yarn lefthook $@
else
echo "Can't find lefthook in PATH"
fi
}
call_lefthook "run pre-commit $@"
My custom `pre-commit` file with `echo` on each "branch"
#!/bin/sh
if [ "$LEFTHOOK" = "0" ]; then
exit 0
fi
if [ -t 1 ] ; then
exec < /dev/tty ; # <- enables interactive shell
fi
dir="$(git rev-parse --show-toplevel)"
call_lefthook()
{
if lefthook -h >/dev/null 2>&1
then
echo "pure lefthook route"
eval lefthook $@
elif test -f "$dir/node_modules/@arkweid/lefthook/bin/index.js"
then
echo "node_modules/@arkweid/lefthook/bin/index.js route"
eval "$dir/node_modules/@arkweid/lefthook/bin/index.js $@"
elif bundle exec lefthook -h >/dev/null 2>&1
then
echo "bundle exec route"
bundle exec lefthook $@
elif npx lefthook -h >/dev/null 2>&1
then
echo "npx route"
npx lefthook $@
elif yarn lefthook -h >/dev/null 2>&1
then
echo "yarn route"
yarn lefthook $@
else
echo "Can't find lefthook in PATH"
fi
}
call_lefthook "run pre-commit $@"
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We already know which binary we want to spawn. index.js
is only needed for npx lefthook
. The new unreleased version automatically downloads the needed binary.
This line should be used when non-npm installations are used.
if lefthook{{.Extension}} -h >/dev/null 2>&1
then
eval lefthook{{.Extension}} $@
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You are saying npx lefthook
is already using node_modules/@arkweid/lefthook/bin/index.js
.
So this check is redundant and should be removed?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes. This PR doesn't solve anything.
index.js is used when running npx lefthook
Line 7 in 6c84d85
"lefthook": "./bin/index.js" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I will update this PR to just remove this useless branch then
Something I just noticed:
In
lefthook/cmd/templates/hook.tmpl
Lines 24 to 29 in 6c84d85
elif npx @arkweid/lefthook -h >/dev/null 2>&1 | |
then | |
npx lefthook $@ | |
elif yarn lefthook -h >/dev/null 2>&1 | |
then | |
yarn lefthook $@ |
I see @arkweid/lefthook
and lefthook
used at the same time
Is it supposed to use @arkweid/lefthook
only?
lefthook
!= @arkweid/lefthook
on npmjs.com
The old optimization was rewritten but left this conditional statement which does nothing now evilmartians#184
020d360
to
da14935
Compare
elif test -f "$dir/node_modules/@arkweid/lefthook/bin/lefthook{{.Extension}}" | ||
then | ||
eval "$dir/node_modules/@arkweid/lefthook/bin/lefthook{{.Extension}} $@" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The executable can be spawned directly. It is slower to spawn lefthook through npx
. Also, removing this assumes dependency on npx, yarn, etc.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I re-read #188 and now I understand this is required
This was for #232
But as @aminya point out
npx
already replacednode_modules/@arkweid/lefthook/bin/lefthook
There is code left which is useless at the point and can be cleanup
So this PR is updated to clean that code up