Hacker News Reader

Comment by skydhash | Hacker News Reader

IMO, in a team settings, improving the review policies and speed has a much better benefit. A PR is supposed to be a proposal for some change, adding more proposals on top of something that is not reviewed is a bit icky.

> . By focusing the stack to the different reviewers you can avoid ambiguity about "what a person is signing off on" in the stack.

That can be easily done with comments. If the PR are orthogonal, they could have been split. And if they're not, I would really like to know how the part that I'm reviewing interacts with the rest of the changes.

View on HN

The PRs may be orthogonal but still be dependent. Feature X depends on improvement Y which also needs bugfix Z. You might go and implement X in a branch, tweaking the codebase as you go, but split the branch apart for review. You put X/Y/Z up, but X contains Y and Z, which means you can't request reviews for X without Y and Z merging, or else have a bunch of extra code that gets in the way.

Let's say that Z has an error (some assumption that does not hold), and needed to be reverted. How does that impact X's viability? I wouldn't trust any reviews of X after that.

I strongly believe that PR should be compared to the main branch, and not rely on unmerged code. Unless you merge everything together in one go. And in the latter case, everything should be reviewed together.

I think the answer is - it depends! This is why we make good money. I don’t think there’s a hard and fast rule here to apply.

If the tooling allows rollbacking X, Y and Z as a set, it could make sense ?

I'm also on the "compare to the main branch" camp in general, but will sometimes end up with a set of X,Y,Z branches that have different purposes but all depend on X.

More often than not, the base dependency is a set of constants or additional class/methods that could be released with no impact (no reference in live code) but still need a somewhat lenghty review process. Reverts would be happening on the higher level PRs, which hopefully are independent.

My experience is that in companies that rely on this flow, these assumptions are generally hashed out during design doc review/discussions. I have never had it happen to me that I need to discard the rest of the stack for an assumption that did not hold. From another angle, if I fear that this might be the case, I hold off on sending parts of the stack to review until I am sure of the assumption holding OR I instead have a "workspace ball of mud" from which I can `split` out well-formed commits. I found this to be an incredibly productive workflow.

> That can be easily done with comments. If the PR are orthogonal, they could have been split.

Comments are ad-hoc and don't scale, relying on the author to interpret and adhere to the extent of the reviewers approval.

> And if they're not, I would really like to know how the part that I'm reviewing interacts with the rest of the changes.

you are free to look up, down, and around the stack; nobody is hiding the code from you. But in many cases this is just unnecessary.

This articulates a source of unease I was having trouble pinpointing. Encouraging piling up changes like this puts pressure on the reviewer to approve things as-is because "i've already built Y and Z on top of X so changing X now is a pain".

In my opinion there's already too much of a bias toward stamping PRs, and organizations that maintain high standards do it partly by resisting that bias in key parts of the codebase so people can push back on things that are poorly engineered or hard to maintain.

I'm strongly on the camp that one PR review shouldn't take more than 10 minutes (vibe number) to review, including reading a linked design spec or speaking with the author for more clarification. Even if I wanted to pull down the branch to test locally, that shouldn't add more than 2 minutes.

Anything that would take a bit longer is either a failure of communication (no design specs, no PR description), a failure of tooling (setting up the project, running tests,...) or a lack of policies (no big changes in a PR).

Reply on news.ycombinator.com