-
Notifications
You must be signed in to change notification settings - Fork 666
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 Prettier GitHub workflow #9163
Conversation
split the difference between the two configs
eslint is a linter that we can run using prettier rules
that I forgot to delete
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 prettier docs discourage using eslint-plugin-prettier
as the method of executing the prettier check: https://prettier.io/docs/en/integrating-with-linters.html
Here's a summary video: https://www.youtube.com/watch?v=Cd-gBxzcsdA
I haven't yet watched the longer talk: https://www.youtube.com/watch?v=sSJBeWPIipQ&t=0s
I think we should first configure prettier in isolation, and secondarily configure ESLint.
When I dug into Prettier, I discovered that the whole point of it is to edit files. This puts it at odds with reporting that there are linting issues as opposed to fixing those linting issues. ESLint is a linter and, in this context, it uses the ruleset established by Prettier as the rules it checks against. I can remove ESLint, but then we're back to changing files rather than simply reporting that there are problems. I could just not commit those changes back to the PR, and report a failure if files were changed by Prettier, but it seems like that maybe defeats the purpose of the tool. |
New idea:
|
I link that idea, and I'd be fine with the same flow for ESLint, I think we can start with just prettier though (up to you if you'd rather remove ESLint or keep it and add a separate action for it (or add on to the same Action)). Back to my comment here: #9082 (comment) I was initially hesitant about CI making the change/commit for us because I feared it'd supplant a learning opportunity. But, I see the potential for learning in this flow
|
swinging back around to this.
I had originally thought of closing this PR and starting fresh, but if we're keeping ESLint (which I think would be a good idea), I'll just back out the ESLint action and replace it. |
Wiz Scan Summary
To detect these findings earlier in the dev lifecycle, try using Wiz Code VS Code Extension. |
and then use those env variables in the pr comment
Example robot comment and change: #9180 (comment) |
so it's easier to understand what it's doing
to remove prettier-plugin
Made the requested changes and the Prettier workflow still seems to be working as expected (see #9180 (comment)). I'm going to close that other test PR since I think I'm done with the testing, this should be good for you to use @stevector. |
This reverts commit 3239aa5.
* Revert "Revert "Add Prettier GitHub workflow (#9163)" (#9254)" This reverts commit eb86890. * trying renaming of config * Revert "trying renaming of config" This reverts commit f823a05. * removing type declaration in package.json * removing eslint * removing eslint * readme fix * trying https://github.com/gatsbyjs/gatsby/issues/9038\#issuecomment-432342005 * Apply Prettier formatting * only update prettier * changing gatsby-node back --------- Co-authored-by: Steve Persch <[email protected]> Co-authored-by: Pantheon Bot <[email protected]>
Fixes #9082
Summary
per #9163 (comment) and #9163 (comment):
npm run lint
script inpackage.json
npm run format
command)*.js
file insrc/
is affected by a pull request and only on the changed files.