-
Notifications
You must be signed in to change notification settings - Fork 134
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
Add husky for pre commit hooks 🐶 #174
base: main
Are you sure you want to change the base?
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: 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.
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
great addition! thanks
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@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.