[HN Gopher] Reorient GitHub Pull Requests Around Changesets
___________________________________________________________________
Reorient GitHub Pull Requests Around Changesets
Author : jamesog
Score : 51 points
Date : 2023-09-30 18:09 UTC (4 hours ago)
(HTM) web link (mitchellh.com)
(TXT) w3m dump (mitchellh.com)
| nh2 wrote:
| A proper review tool such as reviewable.io (which is on top of
| Github) addresses all points in his "problems" list.
| da39a3ee wrote:
| When using changeset-based review, do you find yourself writing
| things like
|
| "This changeset still has the problem I drew attention to in my
| review of the previous changeset. Please see the comment there."
|
| ? I'm just curious how that works; I haven't used changesets much
| but it seems like this would be one inconvenient aspect.
| dundarious wrote:
| Usually you can quickly click through the different changesets
| and see all the old comments. In my experience, it maybe needs
| one more click to follow the reference compared to similar
| comments referencing the current changeset.
| almostnormal wrote:
| Github can show a list of unresolved comments, and with a
| single click jump to the location in the state of the
| specific commit the comment was written at. No need to cycle
| though commits / state at each commit without comments.
| da39a3ee wrote:
| > I'm sure I'm wrong about some detail about some of the points
| above. Someone is likely to say "he could've just done this to
| solve problem 5(a)"
|
| Yep, I'm going to do that!
|
| > Work-in-progress commits towards addressing review feedback
| become visible as soon as the branch is pushed. This forces
| contributors to address all feedback in a single commit, or for
| reviewers to deal with partially-addressed feedback.
|
| This point isn't really valid. It assumes that the contributor
| wants to push their new work addressing feedback to the remote as
| soon as they make each commit. That's OK, so do I (in case my
| laptop disappears or whatever). But, it takes 1 second to make a
| git branch, so you can just push your new commits there and then
| merge or cherry-pick onto the PR branch when you're ready.
| nmadden wrote:
| I did wonder about that when I read it. The idea of changesets
| and versions sounds an awful lot like branches and commits.
| almostnormal wrote:
| If an author is unable to produce readable commits on the
| branch of a PR the author is likely not able to produce
| readable change-sets either.
|
| To review I like branches. Bad commits are not pleasant to look
| at, but they carry information, too, e.g., revealing the need
| for discussion. It's a bit like asynchronous pairing.
|
| I don't think looking at the code on a website is sufficient as
| review in many cases, so I typically have a copy of the branch
| anyway. If any feature is needed, it is the possibiliy to
| comment on unchanged lines.
|
| It probably depends on the scope. If the reviewer works on a
| more abstract level and can rely on authors to get the details
| right, maybe github is not ideal. Where the reviewer is almost
| as deeply involved as the author, a branch works nicely.
| codeapprove wrote:
| I agree 1000%. I'm the creator of what I believe is a better
| review interface for GitHub (https://codeapprove.com) but there
| are also many others: * CodeApprove
| (codeapprove.com) * Graphite (graphite.dev) *
| Reviewable (reviewable.io) * Axolo (axolo.co) *
| Viezly (viezly.com) * Mergeboard (mergeboard.com) *
| Codestream (codestream.com) * Pullpo (pullpo.io) *
| ReviewPad (reviewpad.com) * Planar (useplanar.com) *
| Visibly (visibly.dev) * Codelantis (codelantis.com)
|
| I think in the end we should not expect GitHub to provide the
| best option here. We should expect them to provide a basic option
| (which they do) and for sophisticated consumers to pay more for a
| much better option. Everyone should be shopping for code review
| tools!
| setheron wrote:
| Surprised he didn't refer to stack PR based systems like Gerrit
| for reference.
|
| I have never tried those apps that build a top GitHub. I remember
| Facebook's sapling has a web client as well that does stack
| reviews.
| adm_ wrote:
| I believe sapling uses https://github.com/ezyang/ghstack under
| the hood for stacked PR.
| eyelidlessness wrote:
| There are two mentions of Gerrit in the article. Granted
| neither goes into detail, but both reference it as prior art on
| the topic.
| bittermandel wrote:
| Doesn't he do just that?
|
| > They're already a well-explored user experience problem in
| existing products like Gerrit and Phabricator.
| m3drano wrote:
| IIUC, he's referring to a workflow that Gerrit implements 1-to-1,
| isn't he?
| da39a3ee wrote:
| Yes, he says so two and a half times.
| oldtownroad wrote:
| I agree. As a way to minimise the pain on GitHub today, we
| disallow force pushing and enforce squash merging. Force pushing
| is a nightmarish behaviour, once a Pull Request is opened the
| branch must be append only.
| tmpX7dMeXU wrote:
| I've been contemplating pulling the trigger on this one myself.
| I'm pretty much sold on it. I've been on board with squash
| merging for years. Best thing I've ever done for our project. I
| eventually came to the realisation that the majority of people
| who were against it were so because some purist greybeard had
| beat it into them.
| bhouston wrote:
| This is a great suggestion and likely not that hard for GitHub to
| implement. I am surprised this isn't updated more than it is.
| ninkendo wrote:
| GitHub seems to have no interest whatsoever in making the
| process of reviewing code any better or easier. It's still
| essentially the same as it was 15 years ago.
|
| They introduced batched review comments well over five years
| ago, and still haven't fixed the obvious issues with it, like
| how if you're the PR author and you're replying to someone
| _else's_ comment, and hit Cmd+Enter, it _starts a review of
| your own PR_ (requiring you to delete the comment and start
| over, fun!) rather than just replying. Glaring day-1 oversights
| of what should have been a simple feature, never fixed.
___________________________________________________________________
(page generated 2023-09-30 23:00 UTC)