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

Can we remove npm run test from pre-commit hook? #2274

Closed
Ethan-Arrowood opened this issue Sep 19, 2023 · 7 comments
Closed

Can we remove npm run test from pre-commit hook? #2274

Ethan-Arrowood opened this issue Sep 19, 2023 · 7 comments
Labels
enhancement New feature or request

Comments

@Ethan-Arrowood
Copy link
Collaborator

As titled.

I'm finding it extremely annoying that all of the tests run every time I commit. We should enforce commit squashing anyways when we merge PRs so that not every commit has to be valid.

I currently pass -n every time I commit simply because I don't want to wait for the tests to run, but then when I use npm version and it commits a tag, that hook still runs and there's no way for me to disable it.

Maintainers, WDYT?

@Ethan-Arrowood Ethan-Arrowood added the enhancement New feature or request label Sep 19, 2023
@KhafraDev
Copy link
Member

there was a discussion (on twitter?) a while ago where there seemed to be a consensus that -n was fine and the precommit hook is mostly for new collaborators. I don't see the harm in having it

@Ethan-Arrowood
Copy link
Collaborator Author

I actually sorta remember that discussion - and I may have been on the other side. But now I'm definitely for removing it lol. When other tools trigger the pre-commit hook (npm version) and I can't disable the hook, that's really bothersome.

@metcoder95
Copy link
Member

I've personally never been a fan of git-hooks running as it makes the experience quite annoying when having to commit a change or something; but as well, I tend to use -n or --no-verify (I even make a git alias for it 😅).

If asked, I'm more into removing it but I do not have a problem keeping it with the ability to bypass it 👍

@mcollina
Copy link
Member

I'm happy to follow whatever you prefer.

@ronag
Copy link
Member

ronag commented Sep 20, 2023

I'm fine with removing it as long as it runs in CI with PR

@anonrig
Copy link
Member

anonrig commented Sep 29, 2023

I agree 100%

@metcoder95
Copy link
Member

Closed on by #2296

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

No branches or pull requests

6 participants