Auto-format commits that come through GitHub UI

[pepopowitz]

For contributors who commit through a local environment, we run prettier to auto-format changes via a pre-commit hook. If a contributor makes a commit through the GitHub UI, however, that pre-commit hook does not run. Recent examples: #1089 and 1094.

This often results in unformatted code being committed, and CI failing because it runs prettier to validate the format. This is unfriendly friction for contributors who want to make small, light-weight changes without setting up a local environment.

Implementation

There are a few ways I can think of to address this, but when implementing we are definitely not limited to these options:

1. Add a GitHub action that formats & commits on all PRs/pushes.

   Update: we are unable to configure our repo such that our workflows have write access on forks. See this PR/comment for more details on what we do/do not have permissions to do against a forked repo.

   Out of date comments: When we initially configured prettier, we intentionally avoided doing this because the contributor will not have verified the auto-formatting changes in their local environment. I think my opinion has changed on this, now that we have a pre-commit hook to auto-format for contributors with local environments, and the gap right now is only for people who aren't checking things in a local environment anyway.

2. Add a GitHub action that formats & commits when a PR is merged.

   This does have the potential to introduce changes that no one looked at. This isn't a baseless fear -- here's an example of some auto-formatting that completely broke a code-snippet (the linked Slack comment makes reference to this page).

3. Add a GitHub action that listens for a specific command, and on-demand auto-formats & commits.

   Update: we are unable to configure our repo such that our workflows have write access on forks. See this PR/comment for more details on what we do/do not have permissions to do against a forked repo.

   Out of date comments: This is nice because it only happens when we ask it to....but it seems like it might be a bit more work than the first option.

4. Add a PR review with suggestions for each of the format violations.

   The PR author can then accept the changes through the GitHub UI. We can use this action.

Also in scope

If we find that the solution covers all the right bases, and there is support for removing the pre-commit hook, we should do that as part of this work too.

[akeller]

I like your menu of options here @pepopowitz!

Option 3 is my favorite, mainly because the formatting changes that seem to fail the build the most are the ones associated with tables but also with infrequent docs contributors.

My question for both 1 & 3 is will the action be able to commit to a forked branch unless the contributor specifies? Or would it also fail? That has me leaning more towards 3 over 1.

This doesn't feel like a super high priority to me but definitely an ease-of-use fix that we would want to have eventually. Is this something we could mark as "help wanted" or experiment with accepting community contributors for?

[pepopowitz]

My question for both 1 & 3 is will the action be able to commit to a forked branch unless the contributor specifies? Or would it also fail?

This is a good point -- I'm not sure how well 1 & 3 would work on forks that don't grant access. I do see this comment that indicates that it is possible to commit as a bot to a forked PR....I would definitely want to experiment with this before coming to a conclusion.

My feeling about this is that we should save that experimentation for when we are actively working on this issue. Does that sound right, or do you think it's important for us to know the answer ahead of time?

[pepopowitz]

Oh and then there's an episode of Learn With Jason featuring Brian Douglas on this topic and I'm 100% sure those superstars would have talked about this. I'll watch soon and report back.

[pepopowitz]

Some more notes --

1. It looks like there is a setting we can check that will grant write permissions to the github token used by a workflow running on a fork.
2. Another neat idea that doesn't require write access to the fork/branch -- this action adds a PR review with suggestions for each of the prettier errors. It'd then be up to the PR author to accept the suggestions, which they can do through the GitHub UI 💪

[tmetzke]

That is a great batch of options!

I like option 4 the most as a developer. I stay in charge of my contribution (nothing is autoformatted & committed without me personally checking it first), and I am still responsible for it (I receive suggestions in my PR that I have to accept if I want to get the PR through).

Adding an autoformat commit (on-demand with option 3 or automatically) to a PR is probably also a nice option for a lot of people that don't want to go through a bunch of suggestions in their PR. But if that is too complex to build, I think it already is an improvement to leave suggestions in the PR for those people. I'd prefer that option personally anyways.

[akeller]

Just wanted to chime in here with a pattern I'm noticing among the PM team - this seems to be the de facto way they PR.

I'm not annoyed with this (yet) because I'm happy to do a quick run of prettier and keep their PR up-to-date to keep them unblocked, but if I could do this on-demand or automatically, I would like that more for sure.

[akeller]

That said, I like option 4 the best these days 😄

[akeller]

I have been FIGHTING prettier these days. For small changes I use the GitHub UI in the browser and sometimes I get luck and sometimes I get caught needing to pull the branch locally into VS Code to run prettier.

Is there a way to auto run this in VS Code for every file I touch?

[pepopowitz]

I have been FIGHTING prettier these days. For small changes I use the GitHub UI in the browser and sometimes I get luck and sometimes I get caught needing to pull the branch locally into VS Code to run prettier.

Is there a way to auto run this in VS Code for every file I touch?

I saw that, and it made me cry inside.

I think this command should work -- it's very lightly tested, so it might also not be quite right.

git diff-tree --no-commit-id --name-only XXXXXX -r | xargs npx prettier --write  

where XXXXXX is the commit hash that you want to fix.

Explanation: gets the file names of all changed files in a commit, and sends each one to the command npx prettier --write.

[pepopowitz]

If you wanted to run it for an entire branch, instead of a single commit, I think the front end of the command would change a bit:

git diff --no-commit-id --name-only main -r  | xargs npx prettier --write 

where main is the branch you're comparing to. Again, very lightly tested.

[akeller]

TY, I will test this at some unspecified point in the future!

[pepopowitz]

TY, I will test this at some unspecified point in the future!

I completely over-complicated this, as is my unique skill. Since we keep the docs always-formatted, you can just format the entire project with npm run format. The only changes would be in your current branch, so this would result in no different changes than the complicated mess I shared above.

[akeller]

Ok this sounds much better then 😄