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.

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

Add doc related to setting up pre-commit hook #736

Closed
thelovekesh opened this issue Mar 20, 2022 · 3 comments · Fixed by #747
Closed

Add doc related to setting up pre-commit hook #736

thelovekesh opened this issue Mar 20, 2022 · 3 comments · Fixed by #747
Milestone

Comments

@thelovekesh
Copy link
Collaborator

Currently, we are using lint-staged to run linter on staged files but we are lacking documentation to add lint-staged on the pre-commit hook.
We can add a code snippet in the wiki that can help contributors/devs to set up the pre-commit hook something like this:

echo "set -e; composer run-script pre-commit" > .git/hooks/pre-commit
chmod +x .git/hooks/pre-commit

One more thought came up to my mind. What if the user is on Windows OS?
Should we add husky to overcome this issue? We can add a pre-commit hook using husky on the prepare script. So when the user runs npm install, husky will add a pre-commit hook.

@westonruter
Copy link
Collaborator

Using Husky would work. Or else there can just be Windows-specific instructions.

I'm a bit wary of npm install having such side effects.

@thelovekesh
Copy link
Collaborator Author

I'm a bit wary of npm install having such side effects.

prepare shouldn't have any side effects we need to worry about as a project like Gutenberg also uses it. Also using husky will make sure the pre-commit hook is being used properly. I think we should give it a try 👍🏼

@westonruter
Copy link
Collaborator

By side effects, I mean npm install doing anything other than populating node_modules and writing package-lock.json. But that's also what --ignore-scripts can avoid, so if you want to add husky I'm fine with it.

@thelovekesh thelovekesh mentioned this issue Mar 31, 2022
@westonruter westonruter added this to the 0.7 milestone Mar 31, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants