network icon indicating copy to clipboard operation
network copied to clipboard

chore: Complete Prettier setup

Open mondoreale opened this issue 1 year ago • 4 comments

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

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

mondoreale avatar Jan 08 '25 15:01 mondoreale

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)?

teogeb avatar Jan 08 '25 16:01 teogeb

@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.

mondoreale avatar Jan 09 '25 13:01 mondoreale

@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?

teogeb avatar Jan 09 '25 20:01 teogeb

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.

mondoreale avatar Jan 10 '25 14:01 mondoreale