chore: prettier all files with lint-staged #241
Conversation
nschonni
force-pushed
the
prettier-all
branch
from
1716b4f
to
b3059d5
Compare
|
This LGTM... Are we ok to move forward on it, @xjamundx if the conflicts are resolved? |
nschonni
force-pushed
the
prettier-all
branch
2 times, most recently
from
c059ac8
to
048b86d
Compare
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #241 +/- ##
=========================================
Coverage 100.00% 100.00%
=========================================
Files 25 25
Lines 649 687 +38
Branches 250 260 +10
=========================================
+ Hits 649 687 +38 ☔ View full report in Codecov by Sentry. |
There was a problem hiding this comment.
I believe @xjamundx gave the project to the ESLint Community because he didn't want to be the lead for it anymore.
Especially a non-breaking change like this can be merged once we have eg two reviews or one review and opportunity has been clearly given for others to voice their opinions :)
package.json
Outdated
| "lint": "npm-run-all \"lint:*\"", | ||
| "lint:eslint-docs": "npm run update:eslint-docs && git diff --exit-code", | ||
| "lint:js": "eslint --report-unused-disable-directives .", | ||
| "lint:js": "eslint --report-unused-disable-directives . && prettier --check .", |
There was a problem hiding this comment.
This will not just check JS, it will check everything when it comes to prettier, right?
I think it should be a 'lint:prettier' script that's checked in parallel with eslint (through npm-run-all)
But also:
In the format step the two are applied sequentially and there's nothing that guarantees that the end result style is still prettier compliant after the ESLint fixes has been made – it's quite classic that the two disagrees.
So I'm not sure there should be a prettier --check at all unless we're 100% sure the two will agree on all the things?
There was a problem hiding this comment.
This will not just check JS, it will check everything when it comes to
prettier, right?
Yes
I think it should be a 'lint:prettier' script that's checked in parallel with eslint (through npm-run-all)
I can split it out. This PR was originally from before the lint:* targets were added, and I kept the changes minimal when I rebased it
In the format step the two are applied sequentially and there's nothing that guarantees that the end result style is still prettier compliant after the ESLint fixes has been made – it's quite classic that the two disagrees.
ESLint has been deprecating those style rules since 8.53. Looking at this project, it is using the plugin/config for Prettier, that could be removed and split out to the lint target if you'd prefer
There was a problem hiding this comment.
I pushed a separate commit that strips out the eslint-*-prettier parts and just uses the plain Prettier calls. I left it as a separate commit to make it easier to roll off, if that's not what you were thinking
Maybe you could also put the mention of 2 reviews in the governance doc if that's the approach? |
Its a good idea, but I'm not sure we have enough consencus to get that in yet, its more of a general rule of thumb I would say |
Run for all files in lint-staged, and check on CI through `lint`
Run for all files in lint-staged, and check on CI through
lintWhat is the purpose of this pull request?
What changes did you make? (Give an overview)