-
Notifications
You must be signed in to change notification settings - Fork 19
Conversation
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 requested a couple edits to the README, but otherwise this looks good, and the hook worked correctly for me from the command line. So with those changes, it will be ready to merge.
Thanks for taking this on!
README.md
Outdated
@@ -176,7 +176,7 @@ Run `npm run build` to compile the code to Javascript. | |||
|
|||
Run `node dist/examples/runFasterCode.js` (to use GraphQL) or `node dist/examples/runOldCode.ts` (to use REST calls), to run the example program. It requires internet access, since it calls the GitHub API. It will take a couple minutes to complete. Some of the output includes the word "ERROR", so don't panic. | |||
|
|||
Ensure to lint your code by running `npm run lint` before submitting any code for review. `npm run lint:fix` will automatically fix any errors. | |||
Husky now runs linting checks pre-commit. Either manually fix the errors or run `npm run lint:fix` to automatically fix any errors. |
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 would favor still mentioning how to manually run the linting, in case folks want to do it before attempting a commit.
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.
@redgardner : Also, it looks like there is a bug in husky where sometimes the hooks won't run if you are using an IDE (like vscode in my case) or visual system like SourceTree. We should probably mention that here too, so people realize that the hook not a sure thing. (Which is super frustrating, since that's kind of the point.)
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.
Reference: typicode/husky#639
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.
Oh wow. I had no idea of that issue. How frustrating! Updated this a bit to encourage manually linting and calling out this bug
README.md
Outdated
@@ -176,7 +176,7 @@ Run `npm run build` to compile the code to Javascript. | |||
|
|||
Run `node dist/examples/runFasterCode.js` (to use GraphQL) or `node dist/examples/runOldCode.ts` (to use REST calls), to run the example program. It requires internet access, since it calls the GitHub API. It will take a couple minutes to complete. Some of the output includes the word "ERROR", so don't panic. | |||
|
|||
Husky now runs linting checks pre-commit. Either manually fix the errors or run `npm run lint:fix` to automatically fix any errors. | |||
Ensure to lint your code by running `npm run lint` before submitting any code for review. Either manually fix the errors or run `npm run lint:fix` to automatically fix any errors. Husky set up to run linting checks pre-commit which should prevent being able to commit linting errors; however, There is an [bug](https://github.com/typicode/husky/issues/639) in husky where occasionally the hooks won't run in an IDE. |
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.
@redgardner This looks great. Just a couple minor requests:
- I think the third sentence should say "Husky is set up" (insert the "is").
- It should probably be "a [bug]" rather than "an [bug]".
- In markdown, I have recently developed a preference to split a long paragraph like this into multiple lines, to simplify future diffs. (If that's too much of a pain, I'm wiling to merge it with the current formatting.)
Thanks!
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.
🤦 Sorry for the bad grammar this morning. All fixed up and I think the split paragraphs looks nicer now
No description provided.