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

chore: Complete Prettier setup #2966

Open
wants to merge 5 commits into
base: main
Choose a base branch
from
Open

chore: Complete Prettier setup #2966

wants to merge 5 commits into from

Conversation

mondoreale
Copy link
Contributor

Since .prettierrc is already there… In this PR I complete Prettier's config and make it intertwine with eslint better. Cosmetic change.

Changes

  • Customise Prettier config for better compatibility with Eslint and ongoing conventions.
  • Disable Eslint's indent because it conflicts with Prettier.
  • Rename .prettierrc to .prettierrc.json so it can be formatted using json formatting rules (by Prettier itself).

Limitations and future improvements

  • n/a

Checklist before requesting a review

  • Is this a breaking change? If it is, be clear in summary.
  • Read through code myself one more time.
  • Make sure any and all TODO comments left behind are meant to be left in.
  • Has reasonable passing test coverage?
  • Updated changelog if applicable.
  • Updated documentation if applicable.

@mondoreale mondoreale requested review from harbu and teogeb January 8, 2025 15:14
@mondoreale mondoreale changed the title chore: Complete setting up Prettier chore: Complete Prettier setup Jan 8, 2025
@teogeb
Copy link
Contributor

teogeb commented Jan 8, 2025

Disable Eslint's indent because it conflicts with Prettier.

What do mean by this? Could we configure the eslint rule so that checks that we have the correct 4 space indentation (if it wasn't already checking that)?

@mondoreale
Copy link
Contributor Author

@teogeb, Prettier formats code according to its opinionated rules, and conflicts can arise if ESLint's indent rule tries to enforce a different style. Typically indent is disabled when using Prettier to avoid having to maintain those differences.

@teogeb
Copy link
Contributor

teogeb commented Jan 9, 2025

@teogeb, Prettier formats code according to its opinionated rules, and conflicts can arise if ESLint's indent rule tries to enforce a different style. Typically indent is disabled when using Prettier to avoid having to maintain those differences.

Is the plan the add a separate Prettier task to the CI which checks that the code is formatted using our style? (I.e. that we'd run prettier --check in the CI, in addition to the current npm run eslint.)

If that's the case, it may be ok to remove the duplicated check from eslint. But if we won't add that separate CI task, I'd like to keep the eslint rule so that the indentation is automatically checked by CI.

Another alternative would be to automatically format the code in the CI using Prettier, or otherwise enforce that all code is formatted using it. But that approach has other downsides, and maybe that's not our intention?

@mondoreale
Copy link
Contributor Author

mondoreale commented Jan 10, 2025

It's a bit complicated but yeah, to me the new check seem desirable.

Why complicated? All files would have to be formatted before we introduce the check. Easy in a small project but for us.. well, we have to coordinate because all open PRs *will* suffer a "cosmetic" conflict situation.

One way to apprach open PRs, assuming the main branch has already been formatted globally.

  1. Cherry-pick the new prettier config.
  2. Format all files within the PR.
  3. Merge in main into PR branch.
  4. Carry on.

Well, one can naturally just merge in main and so on. The point is, global changes like this are a bit nasty initially. Luckily it's a one time hassle.

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.

2 participants