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 Prettier GitHub workflow #9163

Merged
merged 25 commits into from
Oct 11, 2024
Merged

Add Prettier GitHub workflow #9163

merged 25 commits into from
Oct 11, 2024

Conversation

jazzsequence
Copy link
Contributor

@jazzsequence jazzsequence commented Aug 8, 2024

Fixes #9082

Summary

per #9163 (comment) and #9163 (comment):

  • Adds/updates ESLint. This was added as part of the initial implementation which has been removed. ESLint was left because we'll probably use it again in the future.
  • Adds a new npm run lint script in package.json
  • Updates the README.md to note how to run ESLint locally or fix all style issues with Prettier (using the existing npm run format command)
  • Updates the Prettier config to use JS-based configuration and splits the difference between the old/existing config and the new config in 4ba6420
  • Adds a new Prettier GitHub action to run the linter when any *.js file in src/ is affected by a pull request and only on the changed files.
    • Prettier-formatted files affected by this are then committed back to the repository by the robot and a comment is left in the PR that files were changed (see test prettier gh action #9180 (comment) for an example)

@jazzsequence jazzsequence changed the title 9082-re-add-prettier Add ESLint to check against Prettier style rules Aug 8, 2024
@jazzsequence jazzsequence marked this pull request as ready for review August 8, 2024 21:18
@jazzsequence jazzsequence requested a review from a team as a code owner August 8, 2024 21:18
@jazzsequence jazzsequence self-assigned this Aug 8, 2024
Copy link
Contributor

@stevector stevector left a 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.

@jazzsequence
Copy link
Contributor Author

jazzsequence commented Aug 13, 2024

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.

@jazzsequence
Copy link
Contributor Author

New idea:

  • remove ESLint
  • run Prettier on PRs. it will change files. the GH action branch will now be ahead of the PR branch.
  • run a git diff against the changes Prettier made
  • post a comment back on the PR saying "these files have linting issues"
  • (optional) maybe output the actual diff in the PR comment as well?

@stevector
Copy link
Contributor

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

  • I'm doing development work in a branch
  • I push/make a PR
  • CI finds and fixes whitespace/etc with a new commit
  • I push again
  • Push rejected because of conflicts
  • I pull and learn

@jazzsequence
Copy link
Contributor Author

jazzsequence commented Aug 13, 2024

swinging back around to this.

  1. Let's keep ESLint. We'll probably use it in the future.
  2. Let's pull the ESLint action out.
  3. Let's add a Prettier action that runs on changed files in PRs
  1. ESLint checks can come after this workflow. In that way, ESLint would check all the things that Prettier couldn't fix automatically, as well as checking already fixed code.

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.

@rachelwhitton rachelwhitton added the Site: Site Structure Issue or PR with the docs app stack and/or live site label Aug 14, 2024
@wiz-inc-b08cf2810f
Copy link

wiz-inc-b08cf2810f bot commented Aug 14, 2024

Wiz Scan Summary

Scan Module Critical High Medium Low Info Total
IaC Misconfigurations 0 0 0 0 0 0
Vulnerabilities 0 13 10 2 0 25
Sensitive Data 0 0 0 0 0 0
Secrets 0 0 0 0 0 0
Total 0 13 10 2 0 25

View scan details in Wiz

To detect these findings earlier in the dev lifecycle, try using Wiz Code VS Code Extension.

@jazzsequence
Copy link
Contributor Author

jazzsequence commented Aug 16, 2024

Example robot comment and change: #9180 (comment)

@jazzsequence jazzsequence changed the title Add ESLint to check against Prettier style rules Add Prettier GitHub workflow Aug 26, 2024
eslint.config.js Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
package.json Outdated Show resolved Hide resolved
@jazzsequence
Copy link
Contributor Author

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.

README.md Outdated Show resolved Hide resolved
@stevector stevector merged commit 3239aa5 into main Oct 11, 2024
8 of 12 checks passed
@stevector stevector deleted the 9082-re-add-prettier branch October 11, 2024 21:02
rachelwhitton added a commit that referenced this pull request Oct 14, 2024
rachelwhitton added a commit that referenced this pull request Oct 14, 2024
rachelwhitton added a commit that referenced this pull request Oct 14, 2024
jazzsequence pushed a commit that referenced this pull request Oct 24, 2024
* 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 <steve.persch@pantheon.io>
Co-authored-by: Pantheon Bot <bot@getpantheon.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Site: Site Structure Issue or PR with the docs app stack and/or live site
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Update and automate the running of Prettier
3 participants