Comment by ninkendo
18 hours ago
> If you do this in a PR, both get assigned to review the whole thing. Each person sees the code that they don't care about, because they're grouped together.
There are two separate issues you’re bringing up:
- Both groups being “assigned” the PR: fixable with code owners files. It’s more elegant than assigning diffs to people: groups of people have ownership over segments of the codebase and are responsible for approving changes to it. Solves the problem way better IMO.
- Both groups “seeing” all the changes: I already said GitHub lets you view single commits during PR review. That is already a solved problem.
And I didn’t even bring up the fact that you can just open a second PR for the frontend change that has the backend commit as the parent. Yes, the second PR is a superset of the first, but we’ve already established that (1) the second change isn’t orthogonal to the first one and can’t be merged independently anyway, and (2) reviewers can select only the commits that are in the frontend range. Generally you just mark the second PR as draft until the first one merges (or do what Gitlab does and mark it as “depends on” the first, which prevents it from merging until the first one is done.) The first PR being merged will instantly make the second PR’s diff collapse to just the unique changes once you rebase/merge in the latest main, too.
All of this is to explain how we can already do pretty much all of this. But in reality, it’s silly to have people review change B if change A hasn’t landed yet. A reviewer from A may completely throw the whole thing out and tell you to start over, or everything could otherwise go back to the drawing board. Making reviewers look at change B before this is done, is a potential for a huge waste of time. But then you may think reviewers from change B may opt to make the whole plan go back to the drawing board too, so what makes A so special? And the answer is it’s both a bad approach: just make the whole thing in one PR, and discuss it holistically. Code owners files are for assigning ownership, and breaking things into separate commits is to help people look at a subset of the changes. (Or just, like, have them click on the folder in the source tree they care about. This is not a problem that needs a whole new code review paradigm.)
> fixable with code owners files.
Code owners automatically assigns reviewers. You still end up in the state where many groups are assigned to the same PR, rather than having independent reviews.
> I already said GitHub lets you view single commits during PR review.
Yes, you can look at them, but your review is still in the context of the full PR.
> And I didn’t even bring up the fact that you can just open a second PR for the frontend change that has the backend commit as the parent.
The feature being discussed here is making this a first-class feature of the platform, much nicer to use. The second PR is "stacked" on top of the first.
> You still end up in the state where many groups are assigned to the same PR
> Yes, you can look at them, but your review is still in the context of the full PR.
Why is this a bad thing? I don’t get it. This has literally never been a problem once in my career. Is the issue that people can’t possibly scroll past another discussion? Or… I seriously am racking my brain trying to imagine why it’s a bad thing to have more than one stakeholder in a discussion.
I can think of a lot of reasons why doing the opposite, and siloing off discussions, leads to disaster. That is something I’ve encountered constantly in my career. We start out running an idea past group A, they iterate, then once we reach a consensus we bring the conclusion to group B and they have concerns. But oh, group A already agreed to this so you need to get on board. So group B feels railroaded. Then more meetings are called and we finally bring all the stakeholders together to discuss, and suddenly hey, group A and B both only had a partial view of the big picture, and why didn’t we all discuss this together in the first place? That’s happened more times in my career than I can count. The number of times group B is mad that they have to move their finger to scroll past what group A is talking about? Exactly zero.
It's totally possible that you aren't the target audience for this sort of feature. It tends to be more useful in very large team and/or monorepo contexts.
This isn't about siloing discussions: it's about focus. You can always see the full stack if you want to go look at the other parts, the key is that you don't have to.
The goal is to get thoroughly reviewed changes. It's much easier to review five 100 line changes than one 500 line one, and it's easier to review five 500 line changes than it is a 2500 line one. Keeping commits small and tightly reviewed leads to better outcomes in the end. Massive PRs lead to rubber stamps of +1.
I agree that that scenario sounds like a nightmare. But I don't think that a PR is the right place to solve that problem: it sounds like something that should have been sorted before any of the code was written in the first place.
7 replies →