Github doesn't notify reviewers / commenters after you force push changes

Development
1

So basically, if you:

  1. Open a PR.
  2. Get a review.
  3. Force push changes.

The people that got themselves involved won’t get updates for this, perhaps not always, I’m not sure. So if you’d like to get feedback after your changes, you can:

  1. Request a review from previous reviewers or a new reviewer here:

ZENIX-selection-2020-12-07T18:42:48+02:00

  1. cc the reviewer with @.
7 Likes
2

@doronbehar: I think it has always been like that and I think that’s a good thing: It allows the author to force push, wait for the CI to build and check the branch, and do some more changes until CI is happy before finally notifying the reviewers.

I think we should always use the reviewers widget because it adds the review to the reviewer’s list of PRs awaiting reviews (i.e., Pull requests · NixOS/nixpkgs · GitHub).

2 Likes
3

I really wish the interaction between notifications and pushes was better documented. I’ve definitely gotten notifications as a reviewer before that just said “We couldn’t find these commits” and had to go back to the discussion tab to figure out what actually happened.

1 Like
4

I’ve been seeing quite a few of those. I assume it’s usually when the author has force-pushed, but the notification linked to an earlier version of the changeset.

If that is indeed what’s happening, it’s really frustrating, given that force-pushing is the only way to update a pull request to a new set of changes while maintaining a sane commit history.

1 Like
5

Is it? Using GitHub’s squash feature while merging of t another way. And it would be great when people suggest changes that we accept in 1 click.

6

Sometimes reviewers are lazy, and sometimes they want to “teach you a lesson”, but I think most of the times if the commit is of more then 2 commits, committers wish to keep the commit history as it should be, and committing changes from suggestions creates new commits which we would like to keep out of the history log.

7

I think @bbigras’s point was that with squash-and-merge in GitHub, you can have your cake and eat it. The PR’s history may be messy (through e.g. accepting suggested changes), but with squash and merge, you’ll end up with one commit anyway (+ a merge commit, but many committers do regular merges anyway).

1 Like
8

I think @bbigras’s point was that with squash-and-merge in GitHub, you can have your cake and eat it. The PR’s history may be messy (through e.g. accepting suggested changes), but with squash and merge, you’ll end up with one commit anyway (+ a merge commit, but many committers do regular merges anyway).

I think some people want some of the PRs to be multiple clean commits, which makes squash-and-merge insufficient.

2 Likes