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

Re: The Theatre of Pull Requests and Code Review

#161

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

roguecoder wrote: post #149

scuff3d wrote: post #128

Earlier quoted context omitted.

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…

That makes sense. Reading your comment got me thinking some of the issue might be that I have always worked on somewhat immature projects. Either R&D or greenfield projects. Which is super nice in a whole lot of ways, but a lot of times I don't know what the final shape of the changes to the rest of the system are going to be, because that part of the system itself isn't well established yet. So it evolves throughout whatever I'm doing. Which would make it difficult to break them off and work them in a different branch.

Maybe there's a partial solution if I can keep those commits clean and separate in the tree. And then when I'm done reorder things such that those all happen as a block of contiguous commits.

Re: The Theatre of Pull Requests and Code Review

#162

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

herval wrote: post #150

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 necess…

The "mid levels who consider themselves senior" are the exact type of people who I see saying what you're saying, i.e.

* Yes, TDD on production code is nice in theory, but it doesnt work in my case.

* Yes, short PRs are nice in theory, but it doesnt work in my case.

In every case, as far as I can see, it meant "It does work, I just dont know how to do it".

When I say "if you dont think it works in your case, come to me, Ill show you" they often demur and I end up with a huge PR anyway.

In practice I dont think ive ever seen a long PR that wouldnt have benefitted from being strategically broken up, but every other day I see another one that should have been.

Re: The Theatre of Pull Requests and Code Review

#163

Post by t-writescode » Thu, Sep 25, 2025, 3:37 PM UTC

nicksergeant wrote: post #107

tcoff91 wrote: post #92

Earlier quoted context omitted.

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.

Yep. Self-reviewing your own PRs is a large boost to both yourself and the team, and often one of the first things I encourage new-ish developers to do. - 90% of the time when you self-review your own PR, you're going to spot a bug or some incorrect assumption you made along the way. Or you'll see an opportunity to clean things up / make it better. - Self-reviewing and annotating your reasons/thought process gives mu…

Even when I worked for myself and had CodeRabbit help me do MRs, I still did a self-review before pushing any change to main.

Self-review is very, very helpful.

Re: The Theatre of Pull Requests and Code Review

#164

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

sexyman48 wrote: post #104

eterm wrote: post #58

Earlier quoted context omitted.

So don't do it in parallel. Completely park other tasks, spend time on the review and record that time appropriately. There's nothing wrong with saying you spent the previous day doing a large review. It's a part of the job, it is "what you're working on".

You might as well go into HR. Everyone knows reviewing other people's PRs is like nurturing their kids at the expense of your own.

Well, then just don't play the game. Make a decision in the team, that everyone accepts everyone's PR immediately without any review. At least you won't have to wait.

Re: The Theatre of Pull Requests and Code Review

#165

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

viraptor wrote: post #22

ValleZ wrote: post #3

If you split all the changes for a feature this way not only you hide the way all changes interact with each other but also make the development at least 10x longer because an average approval time is often more than a day.

> only you hide the way all changes interact with each Splitting the change does not prevent you from looking at diffs of any combination of those commits. (Including whole pr) You're not losing anything. > at least 10x longer because an average approval time is often more than a day. Why do you think it would take longer to review? Got any evidence?

I think approval time would take much longer. The issue is that while actual time spent in review may be shorter, there's a lot of context-switching time costs that increase with the number of PRs submitted.

No one on the team is just sitting there refreshing the list of PRs, ready to pick one up immediately. There's a delay between when the PR is marked as ready and when someone can actually get to it. Everyone is trying to get work done and have some time daily in flow state.

Imagine you have a change; you could do it as one PR that takes 1 hour to review, or 3 small PRs that each take 15 mins to review. The time spent in review may even be shorter for the small PRs, but if each PR has a delay of 1 hour before a reviewer can get to it, then the 3 PRs will take almost 4 hours before they're done, as opposed to just 2 hours for one big PR.

Re: The Theatre of Pull Requests and Code Review

#166

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

goosejuice wrote: post #109

comprev wrote: post #65

Earlier quoted context omitted.

Trust, but verify. We're only human after all :-) At $DAY_JOB we need approvals from peers due to industry regulation.

In my experience, US healthcare, that box can be checked at later stages, namely deployment to production. It's a choice to add it earlier.

If it is for checking a box, sure. If it is part of a process that aspires to deliver projects with quality and with somewhat predictable release dates, that seems way too late, imho.

Re: The Theatre of Pull Requests and Code Review

#167

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

This idea of every PR being a small chunk that you can review in 5-10 minutes is completely ridiculous. It is reasonable for bug fixes or small improvements, but the review time you should expect for a PR should reflect the size and impact of a feature, not some arbitrary number.

Yes, everybody would love it if every PR was small enough. In reality that is not a good way to build substantial features.

Often, fully building out a substantial feature, causes you to change your mind and completely changing your approach the further along you are. You don't want to be muddying up the PR pipeline with a bunch of half-assed changes.

Doing that just makes reviewers less inclined to give good feedback on a PR, because they "know it's going to change so much anyways".

If you are building a substantial feature, it is reasonable that the PR is large and reviewers will have to dedicate substantial time to reviewing it. Reviewing it is work on its own and hopefully your engineers have dedicated time to review substantial features.

Of course, you should make sure your substantial feature is as minimal as possible, for whatever is needed to ship the feaure - but not any less than that.

Re: The Theatre of Pull Requests and Code Review

#168

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

MoreQARespect wrote: post #162

herval wrote: post #150

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 necess…

The "mid levels who consider themselves senior" are the exact type of people who I see saying what you're saying, i.e. * Yes, TDD on production code is nice in theory, but it doesnt work in my case . * Yes, short PRs are nice in theory, but it doesnt work in my case . In every case, as far as I can see, it meant "It does work, I just dont know how to do it". When I say "if you dont think it works in your case, come t…

Sure.

Re: The Theatre of Pull Requests and Code Review

#169

Post by t-writescode » Thu, Sep 25, 2025, 3:39 PM UTC

pards wrote: post #7

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…

> and to ignore my PRs when I don't PRs should be optional, IMHO. Not all changes require peer review, and if we trust our colleagues then we should allow them to merge their branch without wasting time with performative PRs.

SOX compliance audit looks suspiciously at this comment.

No single person being able to make changes to a system is a tenant of that.

Re: The Theatre of Pull Requests and Code Review

#170

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

bob1029 wrote: post #81

I see the primary value of a pull request being the simple awareness of what is being worked on. I aggressively sync my local changes to PRs that are marked draft in GitHub. Other developers I work with do the same. Throughout the day we asynchronously check in on the scope of the others' work. If there is something that looks like it might conflict, we call a meeting. The actual code review phase for me is more abou…

Why do you see feedback as "purity test"ing?

Newspaper reporters are professionals and they still have editors. We've all seen the disaster that results when an author gets too famous to edit. And we don't have to go in and work with their prose later.

Code reviews are where "my" code becomes "our" code: I want my coworkers to feel comfortable with and fully understand and be happy to support the changes I am proposing to our software.