Adds husky precommit for commit and push

[mcdonnelldean]

All commits and pushes now force npm test. Note you can use the -n flag to force up (always handy to have that)

• [x] add flag
• [x] update docs
• [x] add some info to PR for contextual use

Context

Related to #124 This PR only covers the Husky part since this is non controversial. With this PR any commits or pulls will be ran against Unit Tests (not Test Pilot though, that would cause chaos); specifically npm run test.

@noopkat Lemme know if you need any more context, info, or changes.

[coveralls]

Coverage remained the same at 44.84% when pulling 3fe23bafb49465df15c7c8a27386f068ad77507b on mcdonnelldean:add-husky into d9a6dc8a219e6ad2dc9a0e68ae580b5bee500cab on noopkat:master.

[Apidcloud]

How about using opt-in to make it optional?

[coveralls]

Coverage remained the same at 44.84% when pulling 2212c78a812e0fa671e1d9a17194277b41e103e1 on mcdonnelldean:add-husky into d9a6dc8a219e6ad2dc9a0e68ae580b5bee500cab on noopkat:master.

[mcdonnelldean]

@Apidcloud I feel opt-in defeats the purpose of a pre-commit. Consider the point is to run tests when people forget. I do see the point of trying to keep onboarding to a minimum, however I would tackle that by making it almost invisible over giving a choice to turn it on or off.

[Apidcloud]

But people that are not familiar with testing would most likely not be able to commit at all if something breaks. The tests would still be automatically run in the PR.

opt-in would be a way to avoid hindering new contributors.

One could argue that --no-verify flag would suffice, but it could still prove troublesome for new people.

And of course, either way, this information should be added to the Contributing file.

One more thing, is pre-push really needed when there is a pre-commit?

[mcdonnelldean]

@Apidcloud I'm happy for @noopkat to decide on that one over us two. I'm personally a believer in using -n instead of opt-in since it's the "Right Way" were as opt-in obfuscates.

Fully agree on changes to the Contribute.md; I've made some basic changes to let folk know. I am personally against willingly telling folk to use -n or skip precommit in docs, I've found it to be better to tackle that on a case by case basis. In times where they are stuck I would rather they opened an issue than break the tests but this is probably another point @noopkat is best deciding on.

On the last point. That is more professional weariness on my count. I've found times where I've managed to commit, then npm install, then push. Had push not been enabled precommit wouldn't have ran (since it only runs if it gets installed). Now if the tests took an age I would reconsider; but since they are very fast I don't think it does any harm. Open to suggestions on why this may be bad though.

[Apidcloud]

opt-in would make it not run at all by default, whereas the -n is opt-out equivalent. But again, also happy to let @noopkat decide whether it's relevant or not.

I totally forgot about npm installing later. It happens quite often, to be honest 👎

But I didn't fully understand the example. Wouldn't that mean that pre push hook doesn't get installed either? Or does it run without being installed? Not sure since I didn't use it before.

[mcdonnelldean]

@Apidcloud It will only run if you installed so it only covers situations where issues arise between the commit and push (limited but often happen). It won't run if it's not installed (which is kind of a bummer). So it's more just minor edge case handling than anything else.

I suppose you could have it as part of pre-install but that gets messy.

[mcdonnelldean]

@noopkat It might be an idea to hold off merging this based on #124 It may get superseded.