Add husky for pre commit hooks 🐶 #174
Conversation
|
Thanks for opening this PR! We would appreciate it if you could provide us with more info about your PR 🙏 |
|
From now on, whenever we commit a change, Husky checks it against the Husky helps you know whether you're good to go: Or need to get some fixes: cc: @relsteiner, fixed 🍻 |
Codecov Report
@@ Coverage Diff @@
## main #174 +/- ##
=======================================
Coverage 75.20% 75.20%
=======================================
Files 4 4
Lines 125 125
Branches 15 15
=======================================
Hits 94 94
Misses 16 16
Partials 15 15
Flags with carried forward coverage won't be shown. Click here to find out more. Continue to review full report at Codecov.
|
package.json
Outdated
| @@ -18,7 +18,8 @@ | |||
| "copy-templates": "rsync -av --exclude '*node_modules*' ./src/code-templates/ ./.dist/src/code-templates", | |||
| "build:watch": "tsc --watch", | |||
| "start:cli": "node ./.dist/bin/cli.js interactive", | |||
| "publish:build": "npm run build && npm publish ./.dist --access public" | |||
| "publish:build": "npm run build && npm publish ./.dist --access public", | |||
| "prepare": "cd .. && husky install front/.husky" | |||
There was a problem hiding this comment.
Hey, why do you go one folder back and what is the front folder? I don't see this folder 😅
There was a problem hiding this comment.
I know how to fix this, I'll create a PR, we shouldn't only publish the dist folder
There was a problem hiding this comment.
I won't be able to create the PR at the moment so maybe copy the husky script in build for now until I fix this issue
There was a problem hiding this comment.
@rluvaton Can you share your thoughts: What is the failure reason, what would you fix?
@idobetesh Does it fail locally or only in CI? Should indeed install huskey in CI?
There was a problem hiding this comment.
Can you share your thoughts: What is the failure reason, and what would you fix?
the failure reason is that the .git folder is missing in the .dist folder, husky doesn't think to go one folder up as the .dist folder contains the package.json file
Does it fail locally or only in CI
this issue happen both on local and on the CI
Should indeed install huskey in CI?
There are valid use cases adding husky to the CI:
e.g. part of publishing a package we could generate a CHANGELOG file and commit + push it
There was a problem hiding this comment.
About the failure - Publishing to root won't necessary solve it, users who generate code might not have (yet) a git repo. I suggest that the optional solution is thoughtful Husky install command that fails if no Git exists. Can you find one like this please @rluvaton ? Other ideas?
About the need in CI - The idea of Husky is to prevent human mistakes, I don't see why a robot (CI) that publishes docs need another (robot) to check that it didn't forget to run tests
There was a problem hiding this comment.
@idobetesh Awesome contribution, this can trigger early failures for the good
Left one significant comment + CI is failing, we can sort this out with @rluvaton
Let's do it 💪
| @@ -1 +1,15 @@ | |||
| {} | |||
| { | |||
| "arrowParens": "avoid", | |||
There was a problem hiding this comment.
The main selling point of Prettier is "Put aside your personal opinion, avoid team arguments, just use the industry standards that we've set". Should we really adjust its defaults?
package.json
Outdated
| @@ -18,7 +18,8 @@ | |||
| "copy-templates": "rsync -av --exclude '*node_modules*' ./src/code-templates/ ./.dist/src/code-templates", | |||
| "build:watch": "tsc --watch", | |||
| "start:cli": "node ./.dist/bin/cli.js interactive", | |||
| "publish:build": "npm run build && npm publish ./.dist --access public" | |||
| "publish:build": "npm run build && npm publish ./.dist --access public", | |||
| "prepare": "cd .. && husky install front/.husky" | |||
There was a problem hiding this comment.
@rluvaton Can you share your thoughts: What is the failure reason, what would you fix?
@idobetesh Does it fail locally or only in CI? Should indeed install huskey in CI?
No description provided.