chore: Add commit hooks for testing and linting #149
Conversation
|
Coverage after merging chore/commit-hooks into main
Coverage Report
|
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
In this PR I also added the npm script With that I'd like to discuss the scripts we currently have, and if we want to simplify their names, or add common aliases. For example we could change the |
|
Coverage after merging chore/commit-hooks into main
Coverage Report
|
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
Coverage after merging chore/commit-hooks into main
Coverage Report
|
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
Coverage after merging chore/commit-hooks into main
Coverage Report
|
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| @@ -5,6 +5,8 @@ | |||
| "main": "lib/index.js", | |||
| "types": "lib/", | |||
| "scripts": { | |||
| "init-environment": "husky install", | |||
There was a problem hiding this comment.
Could we add this to a postinstal script potentially, so it gets run automatically?
There was a problem hiding this comment.
Based on recommendation from husky I did not add the prepare script to the pre- or post-install steps. Package managers seem to have discouraged this recently. Unless we have a strong opinion, I'd stick with the recommendation from husky. What do you think?
If a contributor forgets this step, they should still be able to contribute. They'll see failures in the PR builds if the violate the linting or tests.
There was a problem hiding this comment.
I'm always a fan of a few manual steps as possible, not a massive problem, but it's an extra step people need to read/act on
There was a problem hiding this comment.
The recommendation from package managers appears sensible on the grounds of better security.
I think that if we do a good job of documenting the setup of the dev environment on the Contributing.md document, and remind the user to lint & test both in that document and in the PR checklist it should be enough.
There was a problem hiding this comment.
in the PR checklist
Our PR builds already lint and test the contributor's code. We can add a checklist step indicating that they should lint&test, but the checklist step is not required to make them aware of failures. I'm not inclined to add a checklist step, as this would repeat work that our PR checks already do. I'm open to adding it, if there's another reason :)
There was a problem hiding this comment.
True. I don't have a good answer though, I think we are all in agreement that the main branch is already protected against untested/un-linted code.
On the other hand we want to favor automation as much as possible, so the happy path here would be that a contributor installs and runs the hooks which in turn would mean these steps of the workflow would pass.
There's a non-zero chance that some contributors will forget to install (and perhaps run) these hooks and in case their code is not linted the workflow will fail. Its not at all a big deal since the contributor will see the error, fix it and commit again.
My reasoning was that having an additional check on the list that says "lint your code (this is done automatically via husky [link to relevant script/explanation])" could help avoid some frustration for some, especially those who are new to all this kind of tooling.
Either way all this is a supposition on my part, am more than happy to not add the item and reconsider if this ever becomes a problem :)
There was a problem hiding this comment.
My reasoning was that having an additional check on the list that says "lint your code (this is done automatically via husky [link to relevant script/explanation])" could help avoid some frustration for some, especially those who are new to all this kind of tooling.
I understand now, great idea! I'll work that in :)
|
Coverage after merging chore/commit-hooks into main
Coverage Report
|
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
Coverage after merging chore/commit-hooks into main
Coverage Report
|
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
Description of your changes
This PR is a proposal for commit hooks. I'm open to discuss and change direction. With commit hooks we can automatically run scripts like linting and tests in various stages of the PR process. This improves the speed at which a contributor gets automated feedback about their change.
With this PR I add linting for pre-commit, and testing for pre-push. This means that code will automatically be linted before creating a commit, and tests will automatically run before pushing the code to origin.
Based on previous experience I added the library husky to manage commit hooks. I decided not to use the default hooks in
.git/hooksas customers might have their own hooks there, e.g. to prevent leaking secrets. Usinghuskyrequires apreparescript that contributors need to run, which I added in the contributing guidelines.Note: If a contributor forgets to run the
preparestep, husky's hooks might not run. As a fallback we have the workflow scripts which execute the lint and test commands. Based on recommendation from husky I did not add thepreparescript to the pre- or post-install steps. Package managers seem to have discouraged this recently.New dev dependencies: husky
Related Issue: #115
How to verify this change
Check out the branch, run
prepare, create a new branch, run git commit, and git push.Related issues, RFCs
#115
PR status
Is this ready for review?: YES
Is it a breaking change?: NO
Checklist
Breaking change checklist
By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.