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

Remove pre-push hooks #242

Closed
daviddias opened this issue Jun 27, 2018 · 8 comments
Closed

Remove pre-push hooks #242

daviddias opened this issue Jun 27, 2018 · 8 comments
Assignees

Comments

@daviddias
Copy link
Member

Everyone gets frustrated using it and most folks just skip it anyways. We now have fast CI so it should be good to remove.

@ghost ghost assigned daviddias Jun 27, 2018
@ghost ghost added the status/in-progress In progress label Jun 27, 2018
@vmx
Copy link
Member

vmx commented Jun 27, 2018

I still find it useful. I would rather disable it on a per-project basis.

@daviddias
Copy link
Member Author

Or rather opt-in than opt-out

@victorb
Copy link
Member

victorb commented Jun 27, 2018

I think the frustration comes from that it takes forever to run. For example, when working on js-multiaddr as the entire suite (node, browser and webworker) takes less than five seconds for me. But on ipfs/js-ipfs it takes 10 minutes to run through the entire thing, so usually I skip it.

Having it be opt-in rather than opt-out sounds like a acceptable compromise though.

@vmx
Copy link
Member

vmx commented Jun 27, 2018

Isn't it opt-in already? If you have a pre-push section in your package.json, then it is run. Else it's not.

@vmx
Copy link
Member

vmx commented Jun 27, 2018

I was wrong. Even without a pre-push section it's running something. I'm happy to look into how to make this opt-in before we remove it completely.

@victorb
Copy link
Member

victorb commented Jun 27, 2018

@vmx afaik, doing the following steps will make my pushes for a project run pre-push stuff before:

git clone $js-ipfs-git-url
cd js-ipfs
yarn
git commit --allow-empty --message "Hello"
git push origin master

Which is opt-out (I need to remove ./.git/hooks/pre-push) rather than opt-in.

@vmx
Copy link
Member

vmx commented Jun 27, 2018

I tried locally the first package that felt sensible. git-validate seems to do exactly what I'd expect. If there is no pre-push section, it does nothing. If there is one (compatible to what we have today), it runs those scripts.

So I propose switching to git-validate and removing pre-push from the projects that don't want to have it. They could even just remove the test and leave the lint in.

vmx added a commit that referenced this issue Jun 27, 2018
The `pre-push` package always runs `npm test` even if there is no `pre-push`
section in your package.json. By switching to the `git-validate` package,
running pre push hooks is completely opt-in. Nothing will be run if there
is no `pre-push` section.

This doesn't change the behaviour of repositories that have some `pre-push`
targets set, as `git-validate` uses the same syntax as `pre-push`.

This change relates to #242.
@jacobheun
Copy link
Contributor

#244 introduced a small npm install bug, ipfs/js-ipfs#1444 (comment)

@ghost ghost removed the status/in-progress In progress label Oct 26, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants