phenomnomnominal / betterer

betterer makes it easier to make incremental improvements to your codebase
MIT License
569 stars 38 forks source link

docs(betterer 📚): suggest a post-rewrite hook in the workflow docs #1186

Open camjackson opened 4 months ago

camjackson commented 4 months ago

Related to #1184.

In PR #1185 I've tried to make the CLI output clearer in the case where files have changed but test results have not.

This PR tries to get at the root cause. In my experience, when this happens, it's almost always because someone rebased their feature branch, pulling in new changes, and then pushed without re-running betterer. They may even have resolved merge conflicts in the results file while rebasing, but really what you need to do is run betterer again after the rebase has finished to make sure it's totally up to date.

I've been able to accomplish that with a post-rewrite hook in my codebase. This PR adds a line to the docs to suggest that as part of the betterer workflow.

Here's what it looks like in the docs site running locally on my laptop:

Screenshot 2024-02-17 at 1 05 05 pm

To complete the picture, this is the post-rewrite hook that I've created in my codebase:

#!/usr/bin/env sh
. "$(dirname -- "$0")/_/husky.sh"

yarn betterer precommit

resultsChanged="$(git diff --staged .betterer.results)"

RED='\033[0;31m'
YLW='\033[1;33m'
NC='\033[0m' # No Color

if [[ -n $resultsChanged ]]
then
  echo
  echo "${RED}The ${YLW}.betterer.results${RED} file has changed after your rebase.${NC}"
  echo "${RED}You need to include these changes in a commit.${NC}"
  echo "${RED}Either create a new commit, or amend them onto your most recent commit.${NC}"
  echo
  exit 1
fi

When it fails it looks like this:

image

phenomnomnominal commented 4 months ago

Nice, I love this, such a good idea to target rebase. Just trying to get my head around the flow and figure out if precommit is the right mode in this hook - the only thing extra that does is basically to run git add. Does that make sense here? I also wonder if they might need to diverge at some point, and maybe there should just be a postrewrite option?

What do you think?

camjackson commented 4 months ago

Yeah, it did occur to me that maybe they need to be treated differently.

The nice thing about precommit is that it happens (wait for it) before the commit is made, so betterer can sneak the results update into the commit via git add as you mentioned. AFAICT, git has no such equivalent for rebasing. The hook runs after the rebase is already done, so you kinda just have to alert the user and tell them to fix it themselves. So that's what my shell script does.

I did consider putting git commit --amend --no-edit --no-verify in the script rather than telling the user to fix it, but it didn't feel right.

Also it's just ocured to me that amending a commit also triggers the post-rewrite hook so you'd end up running betterer again hahaha. From the docs it looks like git does actually pass a flag to the hook which allows you to check if the trigger was an amend or a rebase. I might add a check to my script so that it only runs on a rebase and not on an amend.

Anyway I'm rambling now but broadly I agree that precommit and postrewrite are qualitatively different workflows. Maybe betterer should have a dedicated mode for postrewrite. Rather than git add, it could print out something like the text I put in my shell script. If that's the path you want to go down then I would probably change my other PR to make the CI mode error message a bit clearer and suggest both types of hooks.