Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

camjackson
Copy link

@camjackson camjackson commented Feb 17, 2024

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
Copy link
Owner

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
Copy link
Author

camjackson commented Feb 19, 2024

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants