Skip to content
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 format check to validate #65

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

alexkrolick
Copy link

Fixes #64

What:

Why:

How:

Checklist:

  • Documentation
  • Tests
  • Ready to be merged
  • Added myself to contributors table

@@ -18,6 +18,7 @@ const useDefaultScripts = typeof validateScripts !== 'string'
const scripts = useDefaultScripts
? {
build: ifScript('build', 'npm run build --silent'),
format: precommit ? null : ifScript('format', 'npm run format --list-different'),
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should be: npm run format -- --list-different otherwise the --list-different arg will be passed to npm.

Thanks!

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yarn is not npm

kentcdodds
kentcdodds previously approved these changes Sep 4, 2018
Copy link
Owner

@kentcdodds kentcdodds left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks!

@kentcdodds
Copy link
Owner

Hmm... Do you know why that script is failing? If I try to run it locally on master I don't get a failure. It's odd that it doesn't log what files are causing the failure :-/

@alexkrolick
Copy link
Author

Not sure, can't access the debug logs either

@iamturns
Copy link

iamturns commented Sep 6, 2018

Not sure if this is needed? The eslint config is using eslint-plugin-prettier, which should report any formatting errors.

@kentcdodds
Copy link
Owner

Actually all that does is disable formatting rules which is what we want. I don't want ESLint errors for things prettier will fix automatically. It's super annoying to get red underlines for things I don't need to do anything about.

1 similar comment
@kentcdodds
Copy link
Owner

Actually all that does is disable formatting rules which is what we want. I don't want ESLint errors for things prettier will fix automatically. It's super annoying to get red underlines for things I don't need to do anything about.

@alexkrolick
Copy link
Author

alexkrolick commented Sep 6, 2018

It is true that you wouldn't need this for Javascript if you added eslint-config-prettier and added the prettier/prettier rule, but you would still want it to check the formatting of Markdown and other files (which is where I noticed in the first place).

@kentcdodds
Copy link
Owner

Agreed. I definitely want this 👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

check prettier formatting in validate script
3 participants