chore: add scripts to format/lint only the delta files #305
Conversation
yanick
force-pushed
the
prettier-incremental-lite
branch
from
d747299 to
ba87d38
Compare
| @@ -44,13 +44,19 @@ | |||
| ], | |||
| "scripts": { | |||
| "toc": "doctoc README.md", | |||
| "lint": "(prettier . --check || true) && eslint .", | |||
| "lint": "prettier . --check && eslint .", | |||
There was a problem hiding this comment.
If "unformatted files exist" is an expected state of this repository, then lint shouldn't error if there are unformatted files; I think the || true should be added back to prettier to downgrade it back to warnings
| "lint": "prettier . --check && eslint .", | ||
| "lint:delta": "npm-run-all -p prettier:delta eslint:delta", | ||
| "prettier:delta": "prettier --check `./scripts/changed-files`", | ||
| "eslint:delta": "eslint `./scripts/changed-files`", |
There was a problem hiding this comment.
I can understand the rationale behind incremental prettier, but incremental ESLint goes a step too far in my book. ESLint in our config has most of its formatting checks disabled via eslint-config-prettier, which means its mostly running code quality checks1 ("this should be a const", "var is not in scope," etc.)
Why go out of our way to avoid catching non-stylistic code quality issues?
Footnotes
-
except for the import ordering plugin, but that doesn't feel very high risk for churn given its simplicity / consistency ↩
There was a problem hiding this comment.
I can understand the rationale behind incremental prettier, but incremental ESLint goes a step too far in my book.
Yes, you would do things differently. I got that the first time around.
Listen, we just went through that discussion. At this point in time, I will not have something that force changing large sway of code to appease automatic checks on formatting or linting. Nobody force you to use those delta scripts. You are quite welcome and encouraged to have new code conform to the lint checks. But, and I'll repeat again to be clear, at this point in time I will not have something that force changing large sway of code to appease automatic checks on formatting or linting.
Now, can we consider this particular discussion done, and move to other things?
There was a problem hiding this comment.
My apologies, I got a little unproductively worked up. I get a little passionate about testing practices 😅
There was a problem hiding this comment.
This file conflicts with the ignorePatterns section of .eslintrc.cjs, and it's producing warnings
I added ignorePatterns: ['!/.*'] to ensure .eslintrc.cjs itself would be linted, but that made the assumption ESLint itself was only ever going to be used with . as its argument. That pattern needs to be more specific now
|
Sorry for all the comments on this PR! I just spent a chunk of my morning fixing merge conflicts in my other PRs and kept finding new things that annoyed me. Should've consolidated them into one review. One final note before I get off my soapbox: this PR removes static code quality analysis (aka ESLint) from CI. As a developer, I rely on static analysis almost as much as I rely on my test suites to give me confidence that I can make changes quickly without breaking anything. Given the importance of this library in other people's test suites, I really don't understand why we would choose to make our code quality automation less extensive in this way |
|
🎉 This PR is included in version 4.2.0 🎉 The release is available on: Your semantic-release bot 📦🚀 |
The formatting/linting is not enforced by
validate, but the scripts to format only the files touched by the PR are added for peeps who want to use them.