I was talking to @thibaudcolas and he mentioned the idea of adding formatting for CSS and JS, as well as linting for CSS. I think the easier sell here is the formatter, so I’ll propose that here.
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.
Probably it would be better to do CSS and JS in two separate PRs.
A +1 here too, as Tom mentioned we’ve already discussed this idea quite a bit.
On the process, I’d be interested to hear why you think it’d be better separately for CSS and JS.
Thinking of a few possible things to watch out for,
The ESLint configuration seems to have a few enabled rules about formatting, which might need to be changed to be compatible with Prettier formatting, or need disabling. I’d recommend disabling and leaving all formatting concerns to Prettier.
One of Prettier’s features is adding trailing commas in function calls and parameter definitions, which wasn’t valid syntax until ES2017. That was a while ago now so hopefully not a problem – but something to watch out for if we have automated processes running an outdated JS engine, or static analysis of some kind with an outdated parser.
If Django uses “ignore comments”, we’ll need to check they’re still targeting the correct lines after the reformatting. I only spot a single eslint-disable right now but there could be other tools I’m not aware of.
Prettier is generally pretty fast but it’d be nice to check if there are large folders it might try to crawl looking for files for no reason, and if so ignore those explicitly
As part of merging this, be nice to give a heads’up to code contributors so they’re aware they might see unexpected results / might want to install the relevant code editor integrations.
Just to ease the review burden. I also think CSS might be easier to land (not as many strong opinions around formatting as can come up with a full programming language).
Agreed.
I don’t believe there are.
I think there are only a few directories we need to run this against. Admin, GIS, and tests (didn’t look hard for others yet). We should also not run it against the vendored CSS and JS.
Maybe, but how? Did we do this for black? Don’t recall and wasn’t really involved. Perhaps a follow-up forum post is enough?
As long as the docs cover setting up pre-commit, which is already the case, it should be enough. One of the nice things about formatters is that you don’t have to worry about your own formatting and can just throw code at your editor until it works, then rely on pre-commit to clean it up for you.
A note on how to run it manually might also be useful for some. For example, on ESLint, we have:
Use the ESLint code linter to check your code for bugs and style errors. ESLint will be run when you run the JavaScript tests. We also recommended installing a ESLint plugin in your text editor.
I think something like this should be enough (maybe it can also run with the JS tests).
I think docs for specific editor integrations is probably too difficult to maintain with the shifting landscape and vast amount of editors in popular use.
I don’t think this is so surely. I like to set it up this way myself, but I know a lot of people just write code and rely on pre-commit to do the formatting, rather than battle with editor extensions.
Indeed. (When I was an undergrad, I was taught that Surely doesn’t mean certainly, but rather No argument will be presented for the following claim — I use it rhetorically in the latter sense )
I think docs should show how to install and run the relevant command… for (e.g.) Black…
I count six “+1s” and no other “votes” cast, so I was about to create a ticket for this – but perhaps @tom you have other plans?
I don’t remember whether your participation in a high-profile Django mentoring program has been publicly announced or not – does this work feel like the kind of thing you might want to earmark for a participant to said program?
If not, based on what you said, should we create separate tickets for CSS and JS or just the one?
Another option for JS formatting is Standard JS, which is basically a Black-style “like it or lump it” formatter + linter, implemented as a wrapper around ESLint. I swapped a couple of projects over and like it, and I’m gonna update Boost Your Django DX to recommend it over plain ESLint.
(But I’m happy with Prettier + ESLint for Django too.)
I don’t think Standard is suitable for what we’re discussing here, as we’re after a formatter rather than linting. Standard does have formatting-related linting rules but they’re very limited compared to what Prettier does. And yeah it’s based on ESLint, which itself has deprecated all formatting rules (their formatting rules were created at a time when auto-formatting didn’t exist, and they now recommend using a real formatter instead).
It could be that Standard decides to use the linting rules anyway, extracted as a separate package that they might bundle, but still – those rules are way more limited than what Prettier does.
Okay, so I’ve made a start on this, and have a working configuration.
The problem comes with adding this to pre-commit, which has recently archived the prettier mirror apparently because prettier has broken the ability to use it.
Prettier itself provides some pre-commit options but it would be the first time for us adding something like this.
Of the options available…
Option 1, lint-staged with husky seems reasonable and wouldn’t add too much extra.
Option 2 requires .NET so seems a non-starter.
Option 3 seems okay but is confusing to me.
Option 4 also looks like a non-starter because it lives in the .git directory and can’t be version controlled.
Are there any objections to going with their option 1 here? I don’t really want to have another tool for managing pre-commit hooks, especially when pre-commit is supposed to be able to do everything… but if it can’t, then… I am not sure what good alternative there is.
P.S. looking at Issue with pretier 4.0.0@alpha3 and pre-commit · Issue #15742 · prettier/prettier · GitHub it seems like the pre-commit mirror should still work, the issue that caused the archival was only temporary, so I suppose if someone forked this and updated it, it could be used. Sadly based on his past behaviour I don’t think we can rely on Anthony unarchiving the project any time soon (or ever).
If we don’t want extra tools, I would assume running a local hook would be simpler than options 1 through 3? Here’s an example local hook for Prettier:
# Local hooks that require dependencies to be installed already.
- repo: local
hooks:
- id: prettier
name: prettier
language: system
entry: node_modules/.bin/prettier --write
types_or:
- css
- javascript
Drawback compared to a mirror is this requires having the project’s Node dependencies installed locally. Advantage is no duplication of tooling versions compared to asottile’s pre-commit.
Or we can use a fork of the mirror (existing or make a new one). I’ve been maintaining a stylelint mirror for a while so happy to help / advise.
Indeed. (When I was an undergrad, I was taught that Surely doesn’t mean certainly, but rather No argument will be presented for the following claim — I use it rhetorically in the latter sense 
)
