-
Notifications
You must be signed in to change notification settings - Fork 1.2k
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
Improve the precommit hook #11107
Improve the precommit hook #11107
Conversation
**/review/*.api.md | ||
**/*.yml | ||
**/*.yaml | ||
**/*.d.ts |
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.
Is this necessary?
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'm also curious if this makes a difference
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.
pretty-quick
applies the rules in .prettierignore
file in a way different from prettier
. The former applies the rules relative to the root of source control tree (see here) but the latter applies the rules relative to the ignore path (see here). It is confusing and there is an issue opened for it here: prettier/pretty-quick#95 (comment).
"autoinstallerName": "rush-prettier", | ||
|
||
// This will invoke common/autoinstallers/rush-prettier/node_modules/.bin/pretty-quick | ||
"shellCommand": "pretty-quick --staged --no-restage" |
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.
looks cool!
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.
Looks good! Please wait for another approval before merging.
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.
This is great! Thanks for researching the deep Rush magic for us Deya! 👍
@ramya-rao-a I rebased your branch in this issue #10770 on this one and found the formatting changes applied to be reasonable: |
@ramya-rao-a can you confirm if this change is good for you? I'd love to get it into master soon. |
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.
Let's go!
* Create rush-prettier autoinstaller * Update the hook script to use rush command * do not restage formatted files * update .prettierignore to work with pretty-quick
In this PR:
prettier
wrapper is switched fromprecise-commits
topretty-quick
. The main difference between the two is that the latter works on whole files instead of changed regions only, so no more bizarre formattings. Furthermore,.prettierignore
is picked up correctly and is used to filter out staged files that we do not want to runprettier
on. Fixes Formatting issue with git hook #10770 and fixes [Prettier config] The precommit hook should pick up .prettierignore file #11077.Epic: #11105.