Skip to content
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

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

nschonni
Copy link
Contributor

Was going to start opting the JS/JSX in smaller batches, but found there were some round-trip issues running yarn format and yarn 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.

Copy link
Contributor Author

@nschonni nschonni left a 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

.eslintrc.js Outdated Show resolved Hide resolved
.eslintrc.js Outdated Show resolved Hide resolved
Copy link

codecov bot commented Feb 28, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 85.10%. Comparing base (86500e3) to head (cec1aec).
Report is 235 commits behind head on main.

❗ Current head cec1aec differs from pull request most recent head cdcc055. Consider uploading reports for the commit cdcc055 to get more accurate results

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.
📢 Have feedback on the report? Share it here.

@renchap
Copy link
Sponsor Member

renchap commented Feb 28, 2024

The Prettier plugin is not enabled for the JS/JSX files, so the formatting rules like indent are still useful for those files.

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.

@nschonni
Copy link
Contributor Author

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 !/foldername/*.js to the prettier ignore

Copy link
Contributor

This pull request has merge conflicts that must be resolved before it can be merged.

Copy link
Contributor

This pull request has resolved merge conflicts and is ready for review.

Copy link
Contributor

This pull request has merge conflicts that must be resolved before it can be merged.

Copy link
Contributor

This pull request has resolved merge conflicts and is ready for review.

Copy link
Contributor

This pull request has merge conflicts that must be resolved before it can be merged.

Copy link
Contributor

This pull request has resolved merge conflicts and is ready for review.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants