-
-
Notifications
You must be signed in to change notification settings - Fork 7k
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
Further Prettier ESLint cleanup #29429
base: main
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I included 2 other unused rules at the same time
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #29429 +/- ##
==========================================
+ Coverage 85.01% 85.10% +0.08%
==========================================
Files 1059 1062 +3
Lines 28277 28352 +75
Branches 4538 4548 +10
==========================================
+ Hits 24040 24129 +89
+ Misses 3074 3061 -13
+ Partials 1163 1162 -1 ☔ View full report in Codecov by Sentry. |
82c26cb
to
cec1aec
Compare
The Prettier plugin is not enabled for the JS/JSX files, so the formatting rules like If we enable Prettier for those files (which I am in favour of), it should probably be in one single commit for the whole repository, and not a few files at a time (except maybe for the streaming server). But this will create significant work for our forks to import this change, and require every opened PRs to be rebased, which might be very complex for them. So doing this will require significant documentations to help fork maintainers and PR authors to rebase their changes. This will probably involve them running Prettier on their branch, then rebase against the Prettier-formatted upstream. If you want to tackle this, please do, but even with that I am not 100% certain this will be accepted. |
There are ~160 JS files and ~200 JSX files that change when opted into prettier after this. To make them reviewable, I'd submit them in chunks of a max of 50 files each, buy adding |
This pull request has merge conflicts that must be resolved before it can be merged. |
cec1aec
to
cdcc055
Compare
This pull request has resolved merge conflicts and is ready for review. |
This pull request has merge conflicts that must be resolved before it can be merged. |
cdcc055
to
e2e9732
Compare
This pull request has resolved merge conflicts and is ready for review. |
This pull request has merge conflicts that must be resolved before it can be merged. |
e2e9732
to
008026b
Compare
This pull request has resolved merge conflicts and is ready for review. |
This pull request has merge conflicts that must be resolved before it can be merged. |
I'm not sure if this one should close for same rationale as #26715 (comment) or whether there's something more viable here that could be merged first (conclusion of previous discussion here not clear to me) |
Was going to start opting the JS/JSX in smaller batches, but found there were some round-trip issues running
yarn format
andyarn fix:js
. I cleared out the remaining rules that were disabled in https://github.com/prettier/eslint-config-prettier/blob/main/index.js to go back to the defaults.