Hacker News Reader

Comment by overfeed | Hacker News Reader

> Im fairly sure that I could explain how to break up any long PR in a sensible way. Parent thinks couldnt be done, so do you - what is an example?

Not couldn't - but shouldn't, such as when there's tight coupling across many files/modules. As an example, changing the css classes and rules affecting 20+ components to follow updated branding should be in one big PR[1] for most branching strategies.

Sometimes it's easier to split this into smaller chunks and front-load reviews for PRs into the feature branch, and then merge the big change with no further reviews, which may go against some ham-fisted rule about merging to main. Knowing when to break rules and why, ownership, and caring for the spirit of the law and not just the letter are what separates mid-levels from seniors.

1. Or changeset, if your version control system allows stacking.

View on HN

You can and should break that up because I'm probably going to want to see screenshots to ensure that the branding changes make sense in context and everything looks consistent.

How would you do this? You'd either

1. Create N pull requests then merge all of them together into a big PR that would get merged into mainline at once 2. Do the same thing but do a bit of octopus merging since git merge can take multiple branches as arguments. Since most source control strategies are locked down, this isn't usually something that I can tell my juniors to do

The point of breaking things down like this is to minimize reviewer context. With bigger PRs there's a human tendency to try and hold the whole thing in your head at once, even if parts of the pull request are independent from others.

> The point of breaking things down like this is to minimize reviewer context.

This principle is much more important than some rule that says "Merges to main should not be more than 150 lines long". Sticklers for hard-and-fast rules usually haven't achieved the experience to know that adhering to fundamental principles will occasionally direct you to break the rules.

> Merges to main should not be more than 150 lines long

This can be done by allowing a flag in the commit message that bypasses the 150 line long (or whatever example) rule in the CI that enforces it. Then the reviewers and submitter can agree whether or not it makes sense to bypass the rule for this specific case.

In many cases like this, it's okay to override a rule if the people in charge of keeping the codebase healthy agree it's a special case.

minimizing reviewer context is one thing a PR can try to do, but it's not like that's any kind of universal most-important metric that every PR needs to optimize for, in fact very often minimizing reviewer context is in direct tension with making changes that are holistic and coherent

code review is meant to take time

>Not couldn't - but shouldn't, such as when there's tight coupling across many files/modules.

No, this is a pretty classic example of where you can break up the work by first refactoring out the tightly wound coupling in one PR before making the actual (now simpler/smaller) change in a second PR.

Reply on news.ycombinator.com