Run yarn lint and prettier in CI #1674
Conversation
emdash-ie
force-pushed
the
check-formatting-in-CI
branch
3 times, most recently
from
b7b4bbd
to
d9f2322
Compare
emdash-ie
force-pushed
the
check-formatting-in-CI
branch
from
c0302a7
to
7f6996d
Compare
There was a problem hiding this comment.
The changes look sensible to me, but we're getting a frontend test failure (on the tests for the old config tool) on the latest commit.
These failed for me locally, but re-installing dependencies ( nvm use andnpm i in root, nvm use yarn in ./fronts-client) produced some lock file changes and seems to fix the test
Ah ok! I’ll take a look – I wonder what I’ve done there. Maybe I ran one of the updates on the wrong npm version or with the wrong tool? |
|
I couldn’t reproduce the lock file changes you mentioned locally, but applying the eslint suggestion from the failed test did fix the tests for me locally: hopefully that’ll do? |
|
Hmmm, no: the real errors are the failures to start ChromeHeadless, which don’t happen to me locally but have not been solved. Let’s debug a bit in chat, maybe there’s some difference between my environment and CI. |
emdash-ie
force-pushed
the
check-formatting-in-CI
branch
from
e479f7b
to
6e0804d
Compare
|
The ChromeHeadless failures in the tests appear to be caused by the recent Ubuntu version upgrade in Github’s standard runners. I’ve opened #1688 to fix this separately. |
emdash-ie
force-pushed
the
check-formatting-in-CI
branch
3 times, most recently
from
4fccf9e
to
f81c641
Compare
|
Have flagged the PR to fairground devs as well, since they’re working on this repo at the moment. Will wait to merge to give them a chance to flag any potential issues. |
We’ve had some trouble with different people’s editors disagreeing about formatting, so I’d like to make our formatting checks in CI to have it consistent. I expect this to fail initially, but hopefully it won’t take much work to get it passing.
I was getting a warning that our version of typescript is too new for our version of this package: updating removes the warning.
TSLint is deprecated, and the suggested replacement is typescript-eslint. This commit also updates eslint, prettier, and eslint-plugin-prettier, and adds eslint-config-prettier.
Prettier’s docs [recommend it be run directly rather than via eslint](https://prettier.io/docs/en/integrating-with-linters.html), so this commit removes eslint-plugin-prettier and runs it directly within the lint scripts in package.json.
I’m guessing the path to fronts-client was updated, and the symlink wasn’t.
To produce this diff, I ran `yarn lint-fix`.
The @guardian/prettier config uses tabs for indentation, so this commit changes the .editorconfig files to match. (Since the .prettierrc is linked at the root, I think I need to update both .editorconfig files.)
My editor has some auto-formatting that makes these tables much nicer to read in the markdown source: unless anyone objects I figure it’s an improvement?
- remove mentions of TSLint - recommend setting up editor integration for Prettier
Now that I’ve removed TSLint, this import is failing the build. To fix the build, this commit copies the definition of isError from my local node_modules. I took a look at #1277, which introduced this, and I didn’t see any explanation for this bit of code: it seems strange to import something from tslint (which is a dev dependency) for this, which makes me wonder if it was deliberate? Either way, I believe the check is necessary: I initially tried removing it, but that broke some tests.
Now that we’re not using TSLint, these comments aren’t doing anything. I’ll reintroduce the equivalents for eslint if they’re required.
In a recent commit I ran prettier to format the code, which produced a very big diff. This diff can get in the way of blaming with git, so Iʼve recorded the commit ref in .git-blame-ignore-revs, which facilitates telling git (using the config option blame.ignoreRevsFile) to ignore the commit when blaming. (Iʼll add this config to the setup script shortly.)
This change configures git to ignore the commits listed in .git-blame-ignore-revs when blaming, which prevents formatting commits from obscuring more meaningful blame info.
While I see it, this was being flagged by eslint as a non-breaking warning: might as well follow the suggestion to silence the warning.
emdash-ie
force-pushed
the
check-formatting-in-CI
branch
from
f81c641
to
1bd8e9b
Compare
Got the go-ahead, will merge if/once CI has passed |
|
Seen on PROD (merged by @emdash-ie 7 minutes and 32 seconds ago) Please check your changes! |
What's changed?
We in Content Pipeline have had some trouble with different people’s editors disagreeing about formatting, so I’d like to run formatting checks in CI to keep it consistent.
There’s a little more info in the commit messages, reproduced here:
f1e9664 Run yarn lint in CI
We’ve had some trouble with different people’s editors disagreeing about formatting, so I’d like to make our formatting checks in CI to have it consistent.
5c8fde8 Update @typescript-eslint/parser
I was getting a warning that our version of typescript is too new for our version of this package: updating removes the warning.
b3d894b Replace tslint with typescript-eslint, updates
TSLint is deprecated, and the suggested replacement is typescript-eslint.
This commit also updates eslint, prettier, and eslint-plugin-prettier, and adds eslint-config-prettier.
64dbdbd Run prettier directly rather than via eslint
Prettier’s docs recommend it be run directly rather than via eslint, so this commit removes eslint-plugin-prettier and runs it directly within the lint scripts in package.json.
ed4d99b Use @guardian/prettier for prettier config
6f84c63 Fix .prettierrc symlink in root
I’m guessing the path to fronts-client was updated, and the symlink wasn’t.
2297df1 Format everything with prettier
To produce this diff, I ran
yarn lint-fix.c47958b Update .editorconfig files to match prettier
The @guardian/prettier config uses tabs for indentation, so this commit changes the .editorconfig files to match.
(Since the .prettierrc is linked at the root, I think I need to update both .editorconfig files.)
3714938 Format tables nicely in readme
My editor has some auto-formatting that makes these tables much nicer to read in the markdown source: unless anyone objects I figure it’s an improvement?
9a4f829 Update readme
793db33 Steal isError from tslint
Now that I’ve removed TSLint, this import is failing the build. To fix the build, this commit copies the definition of isError from my local node_modules.
I took a look at #1277, which introduced this, and I didn’t see any explanation for this bit of code: it seems strange to import something from tslint (which is a dev dependency) for this, which makes me wonder if it was deliberate?
Either way, I believe the check is necessary: I initially tried removing it, but that broke some tests.
d9f2322 Remove tslint comments
Now that we’re not using TSLint, these comments aren’t doing anything.
I’ll reintroduce the equivalents for eslint if they’re required.
21bd4aa Add formatting commit to .git-blame-ignore-revs
In a recent commit I ran prettier to format the code, which produced a very big diff. This diff can get in the way of blaming with git, so you can tell git to ignore the commit when blaming using the config option blame.ignoreRevsFile. (Iʼll add this config to the setup script shortly.)
c0302a7 Configure git to use .git-blame-ignore-revs
This change configures git to ignore the commits listed in .git-blame-ignore-revs when blaming, which prevents formatting commits from obscuring more meaningful blame info.
f81c641 Apply eslint suggestion: single quotes
While I see it, this was being flagged by eslint as a non-breaking warning: might as well follow the suggestion to silence the warning.
Implementation notes
@guardian/prettierconfig that I’ve configured prettier to use uses tabs for indentation rather than spaces.Checklist
General
Client