Comment by ninkendo
8 hours ago
> But if those separate parts are large, you really want more structure around it than just a commit.
Why? I reject the notion that large commits should be intrinsically hard to review.
GitHub already has the concept of "code owners", which are people who have ownership/review responsibility over slices of the codebase, based on globs/pattern matching. But they don't implement the other half of that, which is that a reviewer should be able to see a projection of a given PR, which matches the part of the repo they're the owner of.
There. That solves the entire problem of "this is too big, I can't look at all of it" (because your code ownership says this is the chunk of codebase you say you care about), and if that still isn't sufficient, there's a zillion UI features GitHub could add that they simply don't. Why can't I "focus" on a subset of the changes during review, in a way that helps me ignore unrelated discussions/changes? That is, even if I'm not code owner of the `frontend/` folder, why isn't there a UI affordance that says "let me focus on changes inside `frontend/` and ignore discussions/etc for the rest"?
> By using stacked PRs instead of just separate commits, I can add more commits to the individual PRs
Or you could just add commits to the PR, and if GitHub got the damned UI right, it would be natural to see the "slice" you care about, for all the new commits. Having to rearrange commits into separate PR's and slice-and-dice followup changes to file them into the right PR unit, is (to me) a workaround for how shitty GitHub's review UX is. It really shouldn't be this way.
> Then apart from the review process, there are lots of advantages to keeping changes small [...]
I agree with you on most of these points, but the decision to land smaller changes earlier should be made based on things like "let's get early feedback behind a feature flag" or "let's see how this chunk behaves in production so we can validate assumptions", or "let's merge what we have now so to cut back on conflicts", etc. That's all fine. But I'm vehemently opposed to being required to slice up my changes this way, just to work around a terrible review UI.
Personally, I review code in my development environment GitHub's UI is nonsensically terrible to read code. I could go on for hours about this[0], but when looking in my IDE I can drill into a subfolder and look at the diffs there. I can click and follow symbols. I can look at the individual diff history for any wildcarded subset of the repo, and see how the change was broken into commits. If I'm typing up some feedback to say "try doing it this way instead", I can actually try it myself first to make sure I'm not suggesting that someone do something that doesn't even compile.
And GH's discussion UX is by far the worst part of all of it. If you have a thread of discussions around a line of code, then wake up the next morning and want to see what new comments have been added? Good luck. Your best bet is to check your email inbox, because the comments are actually shown to you there. Using GitHub's "inbox" feature? All that is is a link to PR's you have to look at, with no hints at "why" (it could be a CI run finished for all you know.) Good luck figuring out "why" a PR is on your list. Did someone @-mention you? Who knows. So, find the blue dot next to the PR, click it, and then figure out for yourself what changed since the last time you looked. No, you can't just scroll and find it because GitHub hides half the discussions by default. So you have to go and expand all the collapsed sections to hopefully find that conversation you were having yesterday. But oh, you can only find it in the diff tab. So you click that, but the relevant file is collapsed by default ("Large diffs are not rendered blah blah"), so then click that. Then you may find that discussion.
Contrast this to a mailing list. The discussions are... discussion threads. You pick up where you left off. People's comments are right there in your inbox, newest one on top (or whatever your preference is.) You get notified when there's a new message, and when you tap the notification, it's the actual message, not some link to the PR that makes you click 6 more things to maybe find the message that just happened.
[0] like how the first thing you have to do when opening up the changes tab is ctrl+f search for "Large diffs are not rendered by default" to find the actually-important diffs that are not shown to you because GitHub's backend can't scale to rendering large diffs without friction. Countless times I've been burned by approving a PR because I don't see it making a change to some functionality, only to find out it actually did make said change, but GitHub just decided not to show me it. Seriously, the "large diffs" are the most important ones, and those are the ones you don't see without extra clicks. The mind boggles.)
No comments yet
Contribute on Hacker News ↗