The Theatre of Pull Requests and Code Review • Hacker News Board

Re: The Theatre of Pull Requests and Code Review

#141

Post by g-b-r » Thu, Sep 25, 2025, 3:16 PM UTC

mtlynch wrote: post #46

I love code reviews and blog posts about them, but I vehemently disagree with all of this advice. > His example PR[0] adds just 152 lines of code, removes 2 lines, but uses 13 thoughtful commits. > While some developers might understand those 152 lines from the final diff alone, I couldn't confidently approve it without the commit story. This is ridiculous! You absolutely can and should review a PR without demanding…

> Intermediate commits are for the author's benefit, not the reviewer's.

I don't even know how could commits only benefit the author; if they're poor they won't help him either, if not as a log of how much work he's done.

Unless you make a PR for every insignificant change, PRs will most often be composed of series of changes; the individual commits, if crafted carefully, will let you review every step of the work of the author quickly.

And if you don't eschew merges, with commits you can also group series of related modifications.

Re: The Theatre of Pull Requests and Code Review

#142

Post by watwut » Thu, Sep 25, 2025, 3:16 PM UTC

gjgtcbkj wrote: post #37

Every developer I know who applies this sort of “highly documented development” approach where they “work through their thought process openly.” Is only doing it because their thought processes are already so funky and counterintuitive that reviewers actively reject their work unless have written evidence that the developer didn’t just entirely change the scope of their assignment to justify the bizarre decisions.

That is 100% not my experience. People who do that tend to be perfectionists trying to cross all the ts.

Confused developers are just unable to create such reasoning.

Re: The Theatre of Pull Requests and Code Review

#143

Post by pas » Thu, Sep 25, 2025, 3:18 PM UTC

stronglikedan wrote: post #114

dijksterhuis wrote: post #68

Earlier quoted context omitted.

> Minimally, I would like context for the change, … … the why is important > IMO many software developers … don't have explanations to provide. People not taking the time is what makes reviews performative. … a lot of developers only consider the how . i’ve had a lot of experiences of “once my PR is submitted that’s my work/ticket finished” kind of attitude. i spent a year mentoring some people in a gaming community…

> … the why is important > … a lot of developers only consider the how. The why is someone else's job, so the developers should just ask them for a blurb to put in the PR for context, along with a note to the reviewer to ask that person for even more context if necessary.

I think there's a why with regard to the code. Why this "how" and not some other "how". (Why did you pick this algorithm, this pattern, this solution to the bigger business why.)

Re: The Theatre of Pull Requests and Code Review

#144

Post by roguecoder » Thu, Sep 25, 2025, 3:19 PM UTC

davedx wrote: post #41

It's a very common refrain but I don't really agree with it: "How do you create a PR that can be reviewed in 5-10 minutes? By reducing the scope. A full feature should often be multiple PRs. A good rule of thumb is 300 lines of code changes - once you get above 500 lines, you're entering unreviewable territory." The problem with doing this is if you're building something a lot bigger and more complex than 500 lines o…

It's not about splitting up the PR: it is about splitting up the _work_.

If you don't have feature flags, that is step one. Even if you don't have a framework, you can use a Strategy or a configuration parameter to enable/disable the new feature, and still have automated testing with and without your changes.

Re: The Theatre of Pull Requests and Code Review

#145

Post by javier_e06 » Thu, Sep 25, 2025, 3:21 PM UTC

I see Pull Requests and Code Reviews the same way as making a sandwich for someone else. My team wants for me prepare a sandwich ASAP.

They, the reviewers, have to eat it, not me.

Some reviewers want wonder bread bread with a slice of spam.

Some want hand-made spreads with home-grown vegetables.

Is easier to fix a sandwich than a wedding cake.

The take-away

Don't do wedding cakes.

Re: The Theatre of Pull Requests and Code Review

#146

Post by roblh » Thu, Sep 25, 2025, 3:24 PM UTC

Maybe I'm just a scrub, but something that I find makes it harder to do smaller commits is that I frequently rely on being able to see which lines I've changed directly inline in my editor. When you commit, vscode now stops highlighting all of those lines, and that makes it much more difficult for me to orient myself relative to what I've already done. The individual lines, and the git pane that shows which files have been changed, act as waypoints for me while I'm working on stuff. It's particularly important on more complicated features that span more files, and I'll often intentionally commit stuff I feel like I'm not likely to touch again to reduce some visual noise.

Re: The Theatre of Pull Requests and Code Review

#147

Post by DenisM » Thu, Sep 25, 2025, 3:24 PM UTC

WorldMaker wrote: post #118

I wish GitHub's PR UI was better at walking through a PR one commit at a time. As someone who does try to make good commits, and as someone who does try to read PRs sometimes a commit at a time, GitHub's UI gets in the way and keeps trying to drive you back to "whole PR" reviews. It is very telling in the article itself there is a screenshot of the commits tab in the PR workflow that many don't realize even exists an…

Graphite addressees this issue in a different way - it lets the author split a pr into several smaller prs, so one can review it piece by piece, but still see the whole super-pr in one place. It does a decent job rebasing stacked prs as well.

Re: The Theatre of Pull Requests and Code Review

#148

Post by arcanemachiner » Thu, Sep 25, 2025, 3:25 PM UTC

WorldMaker wrote: post #118

I wish GitHub's PR UI was better at walking through a PR one commit at a time. As someone who does try to make good commits, and as someone who does try to read PRs sometimes a commit at a time, GitHub's UI gets in the way and keeps trying to drive you back to "whole PR" reviews. It is very telling in the article itself there is a screenshot of the commits tab in the PR workflow that many don't realize even exists an…

If faced with this issue, I would probably just pull the remote/branch locally and step through it, commit by commit, using my preferred Git manager (Lazygit).

Re: The Theatre of Pull Requests and Code Review

#149

Post by roguecoder » Thu, Sep 25, 2025, 3:25 PM UTC

scuff3d wrote: post #128

sebk wrote: post #73

Earlier quoted context omitted.

I agree with this. One way to keep changes small but still compose them into a coherent PR is to make each commit in the final PR independently meaningful, rather than what actually transpired during local development. TFA touches on this somewhat, contradicting the bit you quoted. A trivial example would be adding the core logic and associated tests in the first commit, and all the remaining scaffolding and ceremony…

The problem that I find myself in is that I almost always run into stuff I didn't expect. Some integration that I thought would be minor turns out to slowly get out of hand, and before I know it I've made way more changes than I meant to. And then it all gets tangled together. Maybe it's just a me problem, maybe I need to be more disciplined. Not sure but it catches me quite often.

That's one of the challenges with making changes all at once: it is a lot easier for one thing going wrong to suddenly result in thousands of lines of changes.

One technique I use when I find that happening is to check out a clean branch, and first make whatever structural change I need to avoid that rabbit hole. That PR is easy to review, because it doesn't change any behavior and there are tests that verify none of my shuffling things around changed how the software behaves (if those tests don't exist, I add them first as their own PR).

Once I've made the change I need to make easy, then the PR for the actual change is easy to review and understand. Which also means the code will be easy to understand when someone reads it down the line. And the test changes in that PR capture exactly how the behavior of the system is changed by the code change.

This skill of how to take big projects and turn them into a series of smaller logical steps is hard. It's not one that gets taught in college. But it lets us grow even large, complex code bases that do complex tasks without getting overwhelmed or lost or tangled up.

Re: The Theatre of Pull Requests and Code Review

#150

Post by herval » Thu, Sep 25, 2025, 3:26 PM UTC

I find the sort of opinions on this post quite common on a subset of engineers - namely mid levels with some time in the career, who start to consider themselves senior engineers and want everyone to follow the same set of strict rules they decided make sense. It’s the same mindset that makes people pedantically apply DRY to every situation or forcing others to TDD basic apps.

In practice:

- smaller PRs aren’t necessarily easier to review (and this arbitrary obsession almost always leads to PR overload in chunks that don’t make any sense, reducing code quality as a result)

- nobody reads intermediate commit messages one by one on a PR, period. I worked on a team where the lead was adamant about this and started to write messages in the vein of “if you’re reading this message, I’ll give u $5”. I never paid anyone a dollar. Don’t waste your time writing stuff for no one.

- “every commit must compile” - again, unnecessary overzealousness. Every commit on the MAIN branch definitely should compile. Wasting your time with this in a branch, as you work towards a solution, is focusing on the wrong thing

You want PRs because they help others absorb what you’re doing (they’ll have to read that same code sooner or later). You don’t want to create a performance theater.