Skip to content
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.

Sign up for GitHub

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

Improve hook template #516

Merged
merged 3 commits into from
Jul 12, 2023
Merged

Improve hook template #516

merged 3 commits into from
Jul 12, 2023

Conversation

hyperupcall
Copy link
Contributor

@hyperupcall hyperupcall commented Jul 11, 2023 •
edited
Loading

Related to #510

⚡ Summary

This improves hook.tmpl by removing extra work and fixing subtle bugs in passing arguments with whitespace.

  • The first commit removes a useless echo (more info can be found here)
  • The second commit removes all "eval" builtins as they are not necessary. $@ has been quoted in all places as should be to properly preserve whitespace.
  • The third commit partially solves lefthook is artificially slow when node_modules is not in the Git root #510. I think it is OK to assume that @evilmartians/lefthook will be executed if npx exists. Cases where it may not may include when a directory has incorrect permissions or the registry cannot be resolved - cases in which the user has a broken configuration already. It is more frustrating when it is artificially slow, especially since npx kind of slow by itself.

☑️ Checklist

  • Check locally
  • Add tests

@hyperupcall hyperupcall changed the title Fix hook template Improve hook template Jul 11, 2023
mrexox
mrexox reviewed Jul 11, 2023
View reviewed changes
internal/templates/hook.tmpl
pnpm lefthook $@
elif npx @evilmartians/lefthook -h >/dev/null 2>&1
pnpm lefthook "$@"
elif command -v npx >/dev/null 2>&1
Copy link
Member

Choose a reason for hiding this comment

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

👍

mrexox
mrexox approved these changes Jul 11, 2023
View reviewed changes
Copy link
Member

@mrexox mrexox left a comment

Choose a reason for hiding this comment

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

Thank you! I will release this PR later today. This change totally makes sense!

@mrexox mrexox merged commit 9930d89 into evilmartians:master Jul 12, 2023
@hyperupcall hyperupcall deleted the fix-hook-template branch July 19, 2023 02:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mrexox mrexox mrexox approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@hyperupcall @mrexox