backdrop / backdrop-issues

Issue tracker for Backdrop core.
144 stars 38 forks source link

Add CSS (JS?) code formatting automation #5568

Open ghost opened 2 years ago

ghost commented 2 years ago

Description of the need

It'd be nice to add an automation (GitHub Action) that formats code nicely for us in PRs. This'd not only make sure code is easy-to-read and consistent, but would prevent the back-and-forth, nit-picky reviews where we ask contributors to add some whitespace here, fix this comment to not overflow 80 characters, etc.

Proposed solution

As discussed in https://github.com/backdrop/backdrop-issues/issues/2785#issuecomment-1081646384, code linters seem to be moving away from 'stylistic' changes and just focusing on flagging/fixing bugs in code. In other words, a linter will warn you about a missing parentheses, but it won't flag whitespace or line-length issues.

This is where code formatters (or 'prettifiers') come in. I recommend we decide on which we'd like to use and then set it up as a GH action to run on PRs.

Alternatives that have been considered

We can always stick with our current system of having a documented coding style, and pointing contributors to that when we mark their PRs as 'needs work', but I don't think that's an ideal solution for reasons I'll get into below...

Draft of feature description for Press Release (1 paragraph at most)

Backdrop now automatically flags and/or fixes code formatting issues in pull requests for readability and consistency.


Related:

ghost commented 2 years ago

Prettier is one option, however it's opinionated, meaning it has its own set of rules by which your code will be formatted and you can't really change them.

Initially this struck me as a bad idea. I mean, we have a set of coding standards that we've developed (inherited), so we'd need an automated code formatter to abide by them, right? But then as I read the Why Prettier? page, I started coming around to the idea of using an opinionated formatter...

Here are the problems of our current system and how something like Prettier would fix that for us:

Like I said, I'm becoming more open to the idea of using an opinionated code formatter, but still open to a discussion about other, customisable formatters if people want to suggest any...

indigoxela commented 2 years ago

This suggestion seems to go a step further than https://github.com/backdrop/backdrop-issues/issues/5245 (where we tried to add a GHA to automatically comment on (but not fix) coding standard violations.

ghost commented 2 years ago

@indigoxela Yep. Coding standards (that affect bugs, functionality, tests, etc.) are different than stylistic changes. These can (and should, IMO) be fixed automatically. Who wants to see tests fail, and have to manually update their PR, because of a missing space, etc.?

bugfolder commented 2 years ago

I love the idea that the nit-picky things like extra spacing or off-by-one indents would be automatically fixed without me having to go back and edit code and do a re-run of tests. But I'd also like to know what I missed, for future reference. Would the GHA issue a report of what it did? Or (even better) make its changes via a commit to the source of the PR, or something like that?

jenlampton commented 2 years ago

@bugfolder I'd like to see it apply a PR to your PR that you then need to review + approve. That way they are automatically fixed, but still needs a human to approve the fixes :)

ghost commented 2 years ago

Here's a PR that adds the 'Prettier' code formatter as a GitHub action: https://github.com/backdrop/backdrop/pull/4035

It just adds a new commit to PRs that cleans up any core CSS/JS files the PR affects.

For example, see my test PR here: https://github.com/BWPanda/backdrop-prettier/pull/3 Someone wants to add some CSS to the Seven theme, but it's badly formatted, so this action cleans it up and adds a new commit (you can see the original commit here: https://github.com/BWPanda/backdrop-prettier/pull/3/commits/63f55c865c6a70c6bdadf126841ebdf6307edc4e).

I know people said above that they'd like the ability to review the changes to their PR before accepting them, but I don't see the need for this. If we decide to use Prettier's coding standards, or even if we find another solution that uses our own coding standards, in what situation would we want a PR against code that decides not to adhere to those standards? If people simply want to see what changes are being made to their PR, they can do so by looking at the commit (e.g. https://github.com/BWPanda/backdrop-prettier/pull/3/commits/d6574fd802a0a38b9469eae53aead11c6bebc7f8).

ghost commented 2 years ago

I think the PR failure is coming from the fact that this PR is based on a forked repo, and there's security around that... See "Use in forks from public repositories" here: https://github.com/stefanzweifel/git-auto-commit-action#advanced-uses

Their recommendation is:

For Workflows which run linters and fixers [...] we recommend running them when a push happens on the main-branch.

This makes sense. Running Prettier on any code pushed to the 1.x branch (as opposed to code in PRs):

So I think I'll try changing the PR here to run Prettier on pushes to 1.x, not on PRs...

indigoxela commented 2 years ago

Oh crap... Isn't that already too late then? Doesn't that also mean that people providing PRs never see, what they could have done better? Core committers already know what things should look like.

ghost commented 2 years ago

I was thinking about this last night, and I realised that it's not ideal having code be formatted only on push and not also in PRs...

Consider this: someone submits some code that is very far away from our standards, formatting-wise. When reviewing their code, how do we decide what needs to be fixed now, and what will be fixed automatically on merge? Over time we may learn what Prettier fixes and what it doesn't, but that's not really ideal. There's a good chance that things we think will be automatically fixed won't be, and then we'll end up with inconsistent code in core (which is the whole point of this issue - trying to avoid that).

So I'll try to do both - have Prettier run on PRs, and on merges to core directly.

ghost commented 2 years ago

Think I have to leave this for now. I can't work out how to get this working in PRs based on a fork (GH permissions seem to get in the way). Maybe someone else will have better luck...