-
Notifications
You must be signed in to change notification settings - Fork 206
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
feat: Add format check to validate script #146
base: main
Are you sure you want to change the base?
Conversation
Codecov Report
@@ Coverage Diff @@
## master #146 +/- ##
=======================================
Coverage 86.86% 86.86%
=======================================
Files 18 18
Lines 335 335
Branches 80 80
=======================================
Hits 291 291
Misses 36 36
Partials 8 8
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.
Quick question.
@@ -18,6 +18,9 @@ const useDefaultScripts = typeof validateScripts !== 'string' | |||
const scripts = useDefaultScripts | |||
? { | |||
build: ifScript('build', 'npm run build --silent'), | |||
format: preCommit | |||
? null | |||
: ifScript('format', 'npm run format -- --check'), |
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.
What happens when prettier is passed --write
and --check
at the same time?
Also, this kinda assumes that the format
script is using something that accepts the --check
flag (like kcd-scripts format
which forwards the flags to prettier). So it probably does, but maybe we should note that in a code comment.
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 test
one also assumes there's a test
script that accepts a --coverage
flag, so I don't think we should mind the --check
flag
Have to look into the code so we don't send the --write
flag when checking I think.
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.
Haven't tested this but the code looks good
Actually just noticed something. Is the script passed file paths to format? If not Prettier will fail and print usage, so you should add |
Closes #64
Closes #65