Precommit checks

[hassankhan]

Hi all, hope everyone's well. Just wanted to gauge opinions on adding pre-commit checks for validation/testing etc? I only ask because getting maintainers' attention for reviews is limited anyway (just the nature of things, right?) and when time is spent nitpicking lint errors, that (IMO) is incredibly wasteful.

We had similar issues in my team, and we resolved them using readily-available tooling on NPM. In all our current projects, we're using husky to execute lint-staged, which is configured to run eslint --fix, run prettier and then run tests for any changed files.

We took this approach instead of mass-reformatting the projects because we didn't want a "style" commit touching every single file. Doing it this way meant as files were touched, they would automatically be reformatted, thus doing it in a more gradual manner.

Any thoughts/suggestions/objections? I can whip up a PR, shouldn't take too long.

[HyperBrain]

Agree, that it is a time-consuming thing having to comment that there are still lint issues in a PR (although Travis already blocks the merge and lets the committers know - and they should fix it). In fact, one should already see it in his favorite editor while writing the code ;-) Same for the coverage and the unit test failures.

instead of mass-reformatting

Fully agree. Style commits for the whole code base create unnecessary conflicts that are even more time-consuming.

Would the pre-commit check mean, that people will get the issues already in their forks before opening a PR?
On our side a check of the PR (or a run of eslint --fix with a staged build on a PR base) could be enough, because noone will commit directly to master.

Additionally, we should add something to the CONTRIBUTING.MD:

Before you open a PR

• Add unit tests for all new code
• run npm run eslint - fix all errors
• run npm test and validate that all tests pass

After you have opened a PR
Set the "ready for review" attribute to YES, only after the automated checks have passed.

Maybe we should also start to compose a MD file here and post ideas for content.

[hassankhan]

Thanks for the reply, @HyperBrain 😄

Would the pre-commit check mean, that people will get the issues already in their forks before opening a PR?

Yep, as soon as someone tries to commit something, the checks run first and if anything fails, the commit does not happen. The contributor would see an error in their Terminal (or Git client) and they'd need to retry the commit.

On our side a check of the PR (or a run of eslint --fix with a staged build on a PR base) could be enough, because noone will commit directly to master.

So we actually do both, we have precommit checks which run on the developer's machine, and then the CI is also set up to run all linters too. We did it this way because contributors would sometimes outright disable git hooks (not understanding their value) and if the CI check wasn't there, then a reviewer would manually have to point out lint errors again.

Additionally, we should add something to the CONTRIBUTING.MD:

Totally agree, but ideally contributors shouldn't need to manually do any of these steps if they set up the project correctly since the Git hooks would be in place.

[hassankhan]

Started a CONTRIBUTING.md here

[hassankhan]

Started a PR here