Live data from Hacker News

The Theatre of Pull Requests and Code Review

meks.quest

81–90 of 431 posts

Re: The Theatre of Pull Requests and Code Review

#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 about making sure the checkin is clean and that what I am intending to work on wont get caught up in a conflicting mess. The code review is NOT a recurring opportunity to purity test my teammates. Presumably, the reason they are working with us in the first place is because they already succeeded at this. "Trust but verify" is a fun trope if you are working somewhere the consequences of a mistake are one-way and measured in millions of dollars. However, a bad commit can be reverted in 10 seconds. Builds of software can be easily recreated. Deploying to production is still sensitive, but why get all weird about rapidly iterating through dev or QA environments?

Re: The Theatre of Pull Requests and Code Review

#82
I feel like quite a lot of the pain is best solved for both sides by inverting the expectations a little towards the author of the PR like this mentions, but around communication, not the substance of the changes itself. Over the years I've built up a bit of a reputation on some teams I've worked on for crafting PRs that are disproportionately easy to review relative to the scope, and it pretty much is entirely from just spending a bit of extra time explaining things in the review comments itself. In addition to the top-level description (which in practice I've doing found often is something people who are understandably busy will just glance through quickly if not skip it entirely to jump right into the diff), I always go through my own code in the review tool without publishing and tend to add a fairly high number of comments explaining things that might stick out as odd in the context of the diff specifically (with comments in the code making more sense for things that will stick out regardless of context). My experience is that for a lot of potential review comments, it's not particularly hard to anticipate just from looking at the diff from the perspective of the reviewer, and it takes far less time as the author to look through and add comments explaining those cases than it does as a reviewer to go through and write up comments on all of those places (especially given that as a reviewer, I do think it makes sense to be thoughtful about how exactly to phrase comments on a PR given how easily tone can be misunderstood over text). My perception is that even going a bit overkill with the self-review doesn't hurt too much; often I'll notice that certain comments get a "thumbs-up" reaction compared to others that don't, which is a nice quick way for reviewers to signify that they understand what I've said and find it reasonable (compared to the comments with no reaction that I assume didn't end up being necessary to address a potential concern).

I picked up this habit from an early teammate (and manager, who eventually went back to just being a teammate because he didn't love being a manager) who recommended it, and in places I've worked where they've had struggles with their review culture, I've had colleagues express to me how much they love that they do this and mention to me that they've sometimes started asking other teammates to do it for certain changes (e.g. "some of this code looks like it might have gotten moved around without changing, but it's not obvious from the diff, do you think you could go through and note wherever that happened?").

At the end of the day, teams will function best when there's mutual good faith and respect for each other's time. (Obviously some teams are lacking this to various degrees, but at that point I don't don't think code review is really the larger problem, but just symptom of the larger underlying dynamic that either needs to somehow be addressed or the team will never work well). Recognizing where you can save your team time overall by spending some of your own is a pretty useful with that in mind, and code review ends up having quite a lot of low-hanging fruit in this regard both because the context that the PR author has tends to make the amount of effort needed to preemptively help the reviewers understand things is quite low compared to the reviewer needing to ask, and because the return on time spent by the author scaling with the number of reviewers.

Re: The Theatre of Pull Requests and Code Review

#83

I've tried his advice several times and it's been a complete failure. Splitting up a PR into a bunch of little PR's causes more problems than it solves, and it makes it 10x harder for the reviewer, no matter how much they complain about long PR's. Now they need to suss out some kind of ordering of the PR's, and navigate between multiple change sets for changes that depend on one another. It doesn't matter how well yo…

This. I have colleagues who helpfully break things into small PRs, but more often than not I wish they hadn’t. I usually want to review things in context of the big picture and that gets lost.

Usually what I do is check out their last PR, figure out what I want to say, and then identify the appropriate place to leave a comment in their stack of PRs. Which is a lot more work for me. And this assumes that they’ve even finished all their PRs instead of expecting them to merge in one at a time

Re: The Theatre of Pull Requests and Code Review

#84
Code review is a good way to ensure 1) the team has context 2) double-check the approach taken 3) keep conceptual integrity high. For that, you should make atomic PRs (not focus on minimal LoC, but rather a coherent patch set for a feature/bugfix), agree with your team that reviewing code is work and set appropriate time aside for it.

Also, code review should be ego free: 1) criticize the code, not the author 2) don’t be too attached to code written, the objective is the product and not number of LoC contributed 3) it’s okay to start from scratch after learning about a better approach, or even make more than one and compare approaches.

Where most teams fail is treating it as a gatekeeping process rather than context sharing, make PRs too small to be meaningful or only waste time arguing about code style and other minutiae.

Re: The Theatre of Pull Requests and Code Review

#85
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…

A stack of PRs is much better for reviewers than a single massive PR.

Use jujutsu and then stacking branches is a breeze

Re: The Theatre of Pull Requests and Code Review

#86
post #63
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…

> You end up doing work on branches of branches, and end up either having to become a rebase ninja or having tons of conflicts as each PR gets merged underneath you +100 to this. My job should be thoughtfully building the solution, not playing around with git rebase for hours.

Just use jj instead of git and cut your rebasing time by 95%.

Suddenly rebasing a stack of branches becomes 1 command.

Re: The Theatre of Pull Requests and Code Review

#87
post #83

I've tried his advice several times and it's been a complete failure. Splitting up a PR into a bunch of little PR's causes more problems than it solves, and it makes it 10x harder for the reviewer, no matter how much they complain about long PR's. Now they need to suss out some kind of ordering of the PR's, and navigate between multiple change sets for changes that depend on one another. It doesn't matter how well yo…

This. I have colleagues who helpfully break things into small PRs, but more often than not I wish they hadn’t. I usually want to review things in context of the big picture and that gets lost. Usually what I do is check out their last PR, figure out what I want to say, and then identify the appropriate place to leave a comment in their stack of PRs. Which is a lot more work for me. And this assumes that they’ve even…

What I find helps with stacked PRs is it helps with getting code review incrementally as the various changes for a larger effort come together.

Re: The Theatre of Pull Requests and Code Review

#88
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…

Hard agree.

Commits are not important. As an author, you should not waste your time on this. As a reviewer, just ignore them.

Re: The Theatre of Pull Requests and Code Review

#89

300 LOC in 10 minutes. Or 2 sec per loc. Or for average 30 char line, 600wpm reading speed. OK. There is little you can review properly in 10 minutes unless you were already pairing on it. You might have time to look for really bad production-breaking red flags maybe. Remember the underlying reasons for PR. Balance between get shit done and operational, quality and tech debt concerns. Depending on what your team need…

Your linter/tests are for catching real errors. Review is to understand the shape of it mostly IMO. I could probably fairly easily review 300 loc if its not a particularly confused shape.

It all depends on the lines. It can take a long time to review a 1 line change to a critical function that is used everywhere in the app and it can take minutes to review 1000 lines of declarative UI code.

Re: The Theatre of Pull Requests and Code Review

#90
post #32
post #14

A lot depends on your goals for your code reviews. And your goals might even be different for different parts of the code base. - Are you trying to make sure that more than one human has seen the code? Then simply reading through a PR in 10 minutes and replying with either a LGTM or a polite version of WTF can be fine. This works if you have a team with good taste and a lot of cleanly isolated modules implementing cl…

I think the other thing that often muddies the waters in discussions of code review is that open source projects and internal codebases are generally in rather different situations. An internal codebase is usually worked on by a fairly small group of experienced people, who are both creating and also reviewing PRs for it. So: - the baseline "can I assume this person knows what they're doing?" level is higher - making…

> - if there's a problem with the code, there's no guarantee that the submitter will be available or interested in fixing it once it's got upstream, so it's more important to catch subtle problems up front

It's also more important to have good tools to analyze subtle problems down the line, thus increasing the importance of bisection and good commit messages.

An underrated benefit of "make it easy for reviewers" is that when a bug is found, everybody becomes a potential reviewer. Thus the benefit does not finish when the PR is merged.

Post reply on HN