Run Prettier and enforce formatting in CI - #6
Conversation
|
natalialuzuriaga
left a comment
natalialuzuriaga
left a comment
There was a problem hiding this comment.
The code and implementation for prettier looks great, the codebase already feels cleaner and more put together.
The last thing blocking from merging is that the Run linter check itself is failing so I'm not able to see the actual update happening in this repo itself. https://github.com/DSACMS/website-template/actions/runs/31630438173/job/94227524502?pr=6
After looking into the error, this is not a result of your change -- it looks like a dependency is outdated and a node version upgrade to 24 is needed to resolve. Up to you to make this addition but no pressure at all to get this fix in because it is your last day. I can easily take it up myself and ensure this gets fixed and merged!
| "dev:assets": "npm run assets:refresh && npm run assets:watch", | ||
| "dev:debug": "DEBUG=* npx @11ty/eleventy --serve --watch", | ||
| "dev:serve": "npx @11ty/eleventy --serve --watch", | ||
| "format": "prettier --write . --log-level warn && git --no-pager diff --color=always", |
| # Liquid / Nunjucks templates. | ||
| # | ||
| # Prettier's HTML parser is not template-aware: it treats `{% ... %}` and | ||
| # `{{ ... }}` as plain text, so it reflows them like prose. That breaks the | ||
| # build -- e.g. it wrapped a Liquid tag's string argument across two lines in | ||
| # _includes/usa_identifier.html, producing invalid syntax. It also cannot parse | ||
| # a dynamic tag name (`<{{ page_title_tag }}>`, _includes/layouts/page.html) or | ||
| # an unquoted attribute holding a Liquid expression (_includes/meta.html). | ||
| # | ||
| # Rendered whitespace is significant here too, and Prettier does not know which | ||
| # of these tags are whitespace-controlling ({%- -%}). | ||
| *.html | ||
| *.njk | ||
|
|
||
| # Build output | ||
| _site/ | ||
|
|
||
| # Generated | ||
| _data/assetPaths.json | ||
|
|
||
| # Managed by npm; reformatting causes churn on every install | ||
| package-lock.json |
There was a problem hiding this comment.
Great file addition!
| # `prettier --check` only lists file names, so re-run it in write mode to | ||
| # print the actual diff. This writes to the runner's temporary workspace | ||
| # only: nothing is committed or pushed, the trigger is `pull_request` (not | ||
| # `pull_request_target`), and GITHUB_TOKEN is read-only. |
There was a problem hiding this comment.
Helpful note here
|
@natalialuzuriaga Bumped up outdated dependency, CI is now green |
2 participants
Problem
Prettier and
.prettierrc.jsonare already in the repo, but nothing runs them, so the config has never actually been applied.Solution
formatandformat:checkscripts so contributors can fix formatting locally withnpm run format.prettier --checkonly lists file names..prettierignorefor the Liquid templates. Prettier's HTML parser doesn't understand Liquid and breaks the build if it formats them.prettier --writeonce across JS, SCSS, MD, JSON and YAML.Also dropped the workflow permissions to
contents: read, since nothing in it writes to the repo or the PR.Result
Unformatted code fails CI and the diff shows up in the job log. Contributors fix it locally and push. 30 files reformatted.
Test Plan
format:check,lintandbuildall pass._siteto confirm the rendered output is unchanged.closes: #5