Opened 3 months ago

Last modified 3 months ago

#35007 assigned Cleanup/optimization

Add prettier to format CSS and JS

Reported by: Tom Carrick Owned by: Tom Carrick
Component: Core (Other) Version: dev
Severity: Normal Keywords:
Cc: Triage Stage: Accepted
Has patch: no Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: no UI/UX: no

Description (last modified by Natalia Bidart)


The idea is to add formatting for CSS and JS.

For motivation, I find myself fairly often in the admin CSS (especially lately) and it’s reasonably laid out for the most part, but there are things that are a little off. Sometimes the indentation is different from the rest of the file, for example. The JS is in a bit worse shape I think, some of the files have whitespace issues. These could be fixed but it would be better to enforce them in a way that’s easier to review, so I think a formatter makes sense here. What’s annoying (for me) is that when editing files, sometimes my editor “fixes” the issues for me, then I have to revert them and save without formatting. If I don’t spot my editor’s work, it’s more annoying to fix. Of course I could turn this stuff off somewhere, but if it’s annoying for me I’m sure it’ll be annoying for others as well.

I’m open to suggestions, but Thibaud suggested prettier, and I also use it myself. It doesn’t really require any configuration, though is configurable if people do have strong opinions.

But barring differing opinions there, the process would be something like:

Make a PR with three commits. The first sets up the machinery, installs prettier, adds to pre-commit, etc. The second does the work and the third adds the second commit to .git-blame-ignore-revs. As far as I know, the same as when black was set up.

Change History (5)

comment:1 by Tom Carrick, 3 months ago

Owner: changed from nobody to Tom Carrick
Status: newassigned

comment:2 by Natalia Bidart, 3 months ago

Description: modified (diff)
Triage Stage: UnreviewedAccepted

The forum topic has had very positive reactions, accepting.

comment:3 by Mariusz Felisiak, 3 months ago

We're already using ESLint for JS files.

comment:4 by Tom Carrick, 3 months ago

We can have some discussion about whether ESLint's formatting rules are enough, but I don't think they are (at least the current ones). For example, they don't seem to catch obvious whitespace issues (whitespace only lines in SelectFilter2.js, whitespace at end of line in RelatedObjectLookups.js I found in the first two files I opened, so there are probably others).

It was suggested in the forum post to use ESLint only for linting, and use prettier for formatting, and I think this is a good idea.

Last edited 3 months ago by Tom Carrick (previous) (diff)

comment:5 by Thibaud Colas, 3 months ago

ESLint’s formatting rules definitely aren’t enough. It’s more "auto-fix white-space in a few very specific scenarios" than "format everything". ESLint themselves have also deprecated their formatting rules and recommend using a dedicated formatter.

Note: See TracTickets for help on using tickets.
Back to Top