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

Re: The Theatre of Pull Requests and Code Review

#91

Post by ambicapter » Thu, Sep 25, 2025, 2:15 PM UTC

xmcqdpt2 wrote: post #44

jvanderbot wrote: post #5

PR review is probably at least a little performative. But I trust my colleagues to do good reviews when I ask them to, and to ignore my PRs when I don't. That's kind of the way we all want it. I regularly ask for a review of specific changes by tagging them in a comment on the lines in question, with a description of the implications and a direct question that they can answer. This, "throw the code at the wall for in…

This is a huge pet peeve of mine. At work I'm an expert on part of the code base that sees a fair number of contributions.I get many private IMs from colleagues asking me to "Approve please" or something like that with a dump of 100s of lines, of which maybe 10 lines are relevant to me (touch files or behaviour that I'm an expert on, hence why they need my approval.) Minimally, I would like context for the change, wh…

> IMO many software developers are just not fast enough at writing or language

I think this is the overwhelming factor, software engineering doesn't select for communication skills (and plenty of SEs will mock that as a field of study), or at least most SEs don't start out with them.

Re: The Theatre of Pull Requests and Code Review

#92

Post by tcoff91 » Thu, Sep 25, 2025, 2:15 PM UTC

vinnymac wrote: post #70

jvanderbot wrote: post #5

PR review is probably at least a little performative. But I trust my colleagues to do good reviews when I ask them to, and to ignore my PRs when I don't. That's kind of the way we all want it. I regularly ask for a review of specific changes by tagging them in a comment on the lines in question, with a description of the implications and a direct question that they can answer. This, "throw the code at the wall for in…

For any PR above a few line change, if a developer has not done a self review, I don’t review it all. Instead I request that it is self reviewed with context added, prior to requesting re-review. I also tend to ask the question, “are any of these insights worth adding as comments directly to the code?” 9/10 the context they wrote down should be well thought out comments in the code itself. People are incredibly lazy…

By self review, you mean that the developer adds comments in the code review tool? that is a great idea, I want to try this.

Re: The Theatre of Pull Requests and Code Review

#93

Post by Dan42 » Thu, Sep 25, 2025, 2:15 PM UTC

I see posts like this one pop up from time to time. I love it. Based on my 30y of exp that's also the workflow I converged on. It seems to me like every experienced and skilled developer is converging on this. jujutsu is entirely built to accommodate this workflow.

There are no silver bullets or magical solutions, but this is as close to one as I've ever seen. A true "best practice" distilled from the accumulated experience of our field, not from someone with something to sell.

Re: The Theatre of Pull Requests and Code Review

#94

Post by bonzini » Thu, Sep 25, 2025, 2:17 PM UTC

ziml77 wrote: post #39

smashedtoatoms wrote: post #28

Earlier quoted context omitted.

I'm editing this to be nicer. I'm really trying to be nicer. Consider the possibility you're not the only one in the codebase and that the git history might provide the why to the code's what.

As a reviewer, I don't care how you got to the end result. I want to see the final code. If you settled on something in your code that was unintuitive, because you tried simpler ideas that didn't work, then note that in a comment. Comments provide the info inline and don't require someone reviewing the code now or working on the code 15 years later to read your "commit story" to understand it.

You confuse "include all the broken work-in-progress commits" with "split the independent parts that get you from A to B".

Re: The Theatre of Pull Requests and Code Review

#95

Post by bonzini » Thu, Sep 25, 2025, 2:18 PM UTC

karel-3d wrote: post #64

If you want to let the review in, you need to make some obvious issues - typo or space/tab thing - so you give the reviewer some bone so they feel like they did something and accept your request otherwise.

Kinda like "the queen's duck" (https://bwiggs.com/notebook/queens-duck/).

Re: The Theatre of Pull Requests and Code Review

#96

Post by SAI_Peregrinus » Thu, Sep 25, 2025, 2:20 PM UTC

fidotron wrote: post #50

StilesCrisis wrote: post #36

Earlier quoted context omitted.

The goal here is to make almost all CLs trivial enough that they can be reviewed quickly. You can compose almost any feature out of many small simple changes.

> You can compose almost any feature out of many small simple changes. You can?

Yes, and you can waste a ton of a coworker's time doing it.

Say you're upgrading to a new library, it has a breaking change to an API. First, add `#if`s or similar around each existing change to the API that check for the existing library vs the new version, and error if the new version is found. No behavior change, one line of code, trivial PR. One PR per change. Bug your coworker for each.

Next, add calls to the new API, but don't actually change to use them (the `#if`s won't hit that condition). Another stack of trivial PRs. Your coworker now hates you.

Finally, swap the version over. Hopefully you tested this. Then make a final PR to do the actual upgrade.

For something less trivial than a single breaking upgrade, you can do the same shit. Conditionally compile so that your code doesn't actually get used in the version you do the PR in, you can split out to one PR per character changed! It'll be horrible, everyone will hate you, but you can split changes down to ridiculously small sizes.

Re: The Theatre of Pull Requests and Code Review

#97

Post by sexyman48 » Thu, Sep 25, 2025, 2:22 PM UTC

gjgtcbkj wrote: post #54

octo888 wrote: post #30

What I've often found is that people only really accept feedback from the Tech Lead, and peers are dismissed (not outright and not obviously - kind of sealioning etc). Peer-to-peer code reviews are another instance implementing a thing that pretends hierarchy does not exist. You can only get basic tweaks accepted. The sunk-cost fallacy is a huge force. Maybe I've only worked at crappy places

Allen Iverson got criticized by the media for “letting down his teammates” for skipping a few practices. He famously said “We’re talking about practice, practice! not the game! How is MY going to practice gonna make THEM better?!” He got flack for those comments, but what he said was accurate. If we’re talking about outcomes you’re beholden to the person who is seen as the difference maker, all the teamwork in the wo…

Every company depends on 3-5 guys for their competitive advantage. While the other thousand guys are interchangeable, they're just as necessary (and needful) to the company's survival.

Re: The Theatre of Pull Requests and Code Review

#98

Post by devmor » Thu, Sep 25, 2025, 2:23 PM UTC

I actually didn't ever realize that some people dreaded code reviews. To me, PRs are one of the most exciting parts of the process. That's where you have the most potential to learn from your colleagues and/or teach them something you know.

Re: The Theatre of Pull Requests and Code Review

#99

Post by parpfish » Thu, Sep 25, 2025, 2:25 PM UTC

i don't know how people can build something and leave behind a coherent series of commits that tell a nice story of progressively building a thing.

my commits include lots of false-starts that get abandoned and "i need to commit this interim state because i deprioritized this and will come back later".

the sequence of events that i used to build the thing isn't necessarily the best sequence of events to tell the story of what the final thing is.

sometimes you can get a good story by stacking PRs, but if the stack gets too deep you can end up with some rebase nightmares.

Re: The Theatre of Pull Requests and Code Review

#100

Post by nerdponx » Thu, Sep 25, 2025, 2:25 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…

Here's an alternative approach: Discuss the design with your team beforehand, and have active ongoing discussions, sanity checks, and even pair programming during the development process. That way the review is not an exhaustive end-to-end review with the reviewer coming in cold. It's instead the final approval step in a long chain of decisions that have already been discussed and agreed upon.

Of course that won't work for all projects/teams/organizations. But I've found that it works pretty well in the kinds of projects/teams/organizations I've personally been a part of and contributed to.