Ask HN: What is a common PR review time at your company?

As a reviewer... it really, really depends on how trivial or gnarly the PR is.

If it's a simple change that is obviously correct, I'll try to unblock the author ASAP - often within minutes, even if it interrupts my flow.

If it's a giant PR with lots of risky changes in vital code, an awkward-to-unreadable diff, and/or maddeningly questionable design/coding decisions that require me to think a lot and compose some nuanced, reasoned comments to steer you in a better direction, then, well, you might find yourself nagging me for a review 2 days later. (And I'll probably ask you to split up PRs like this in future, e.g. to separate major refactoring from any logic changes.)

Here's what no one will tell you about PR reviews. Highly productive engineers often learn to work around them. Once you've been around for a while, you gain trust. This usually results in a group of people who will rubber stamp anything you send them.

I've been doing this for 20 years and I've never seen an organization where this didn't exist to a significant degree.

I review my PRs myself line by line. And because of this I've literally never had a PR reviewer catch a major bug (a bug that would have required declaring an incident and paging an on call engineer had it made it through to prod). Not a single time.

So the vast majority of the time, I don't care that someone is going to rubber-stamp my PR. Occasionally, I'll ask someone who knows more than me to give it a thorough going over just in case.

I also very rarely have someone suggest a change in PR that was worth bringing up. But most of the time if I'm doing anything non-trivial, I've already talked it over with someone else well before PR time. I think that is the real time when collaboration should be happening, not at the end of the process.

Essentially, the PR a process at most tech companies is nothing more than a gatekeeping/complicance/CYA mechanism, and realizing this can definitely help your career..

Code review should really only exist to guide contributors to the owner of the code they are changing. A new contributor shouldn't need to know the team structure and history of the organization to get changes in. That should be solved by the tooling. Do whatever you want, and the tools will force you to bump into the right people before you can merge.

It's very strange that most companies (maybe GitHub is to blame for this) have something like a free-for-all where anyone can approve anything, as long as they themselves aren't the author. As other comments have mentioned, this reliably creates reviewer cliques as engineers seek out the path of least resistance to get changes in. Those who haven't figured out the game often lose hours of their time to nitpickers and pedants without anything better to do.

This also results in a slow decay since most of the code ends up being written by people who are good at getting code merged, and not people who are good at building the software.

At the last place I worked it was common to wait one to two weeks for a PR review. I was the only developer who would do a review within the same day that someone requested it; as a result everyone came to me when they needed a review to actually get done.

When performance reviews came around I was criticized for not getting enough story points merged (because I couldn't get them reviewed) and the fact that I was single handedly responsible for reviewing almost all of the PRs was not deemed to be a valuable contribution to the team.

Inside my team, we generally review quite promptly (same day, or the next morning). Across teams — it's more variable and depends a lot on context (or lack of context). If I'm chasing down reviews, I'll usually tag people to start out, and then message them directly to ask for reviews 24 hours later, if they don't get back to me sooner. It's rare that someone doesn't review same-day if I ask them directly.

I often find the reviews to be helpful. People come up with edge cases I haven't thought of, or notice things to clean up. I try to proofread everything before asking for reviews, but after I've started at my own diff for a while, it's hard to read it with fresh eyes.

Sometimes we ask for reviews on more experimental PRs too, just to get early design feedback before wasting too much time on polishing the changes.

If your PR isn't blocking someone nontechnical then I expect you to use developer tooling (i.e. VCS) to continue working without requiring that your PR be merged immediately, and I'll review the PR the next time I come to a natural break in my work. If it is blocking someone, then I'll review ASAP with consideration to the priority of whatever I'm working on.

I work with a guy who, until I said "calm down", would share his PR links in the team chat several times a day. He refused to learn rebase, or seemingly anything other than a standard Git workflow of checkout, commit, push, merge, repeat. He has since improved on both fronts.

We expect same day or next day response. Of course only if you're in. But if you're not I might reassign. Also if it blocks me I will ping you in less than a day or straight away.

My most common response is "needs no further review if these points are addressed". On rare occasions my only response is "need more time". Some changes take time to settle in my mind. No good comes from rushed comments.

There is an expectation not to haggle over dumb shit. When you decide to pick a battle, pull in a second reviewer for their opinion.

I'm sure you've heard the "trick" of splitting up large merges to make them easier. Yes this is good up to a point. Other than that, it really is team commitment to timely reviews. No tricks to it.

It depends on the specific coworker I send it to. I have coworkers who will review within an hour without any additional prodding, and coworkers who will take up to a week unless I prod.

In general for small PRs I consider a consistent latency of over 24 hours to be long and mention it negatively in peer review.

Our standard is that when a reviewer is no longer in flow, review should be the next thing they do. The SLO is 1 business day, which is supposed to be the maximum, and the guidance suggests it should be far lower than that. In reality, it varies wildly by team.

I find most of my PRs being reviewed by my team, on the same timezone, are reviewed within a few hours, but I suspect a big part of that is that most of my PRs are very small (they're actually CLs not PRs, and closer to a single commit in some ways, although hard to draw specific parallels to Git).

It depends on the language/ecosystem a bit, but normally I'd try to keep changes under 100 lines for an efficient PR process. Up to 500 lines is manageable but much slower. Over that is just impossible to do good review on.

Something that my previous company had a lot of success with was review by commit. Making sure that the series of commits in a PR "tells a story", with each one being simple and obvious, and then instructing reviewers to review commit by commit. It can speed things up and increase the quality of review substantially, but does require a bit more work from the author.

Has heavily depended on employee incentives.

At some employers, particularly the Scrum point counting ones, PRs were unrecognized work, so everyone avoided them unless hounded and then people traded PR approvals at the end to score more points as “done.”

At employers that didn’t care about points complete per sprint, it depended on the overall importance of the work as the team leads jumped in to have them done. But even then, as it wasn’t recognized as work in any way, during crunch it got abandoned.

I've always established the team rule that most PRs must be reviewed within a day. At the very least, devs should be reviewing any open PRs before they leave or when they first get in.

Obviously, there's going to be exceptions for large changes or big rewrites on the one hand, or for tiny label changes on the other hand.

PS. Reading the comments here, people seem to work in some pretty dysfunctional environments. Get the team together and establish some guidelines that everyone agrees on. Review time should be predictable, and the work should be reasonably shared.

We've gotten to the point at my company where PRs are almost a formality. PR -> merge -> Put on environment for QA. QA finds issues, fix issues with new PR, merge, and so on until we have our release.

My last company was much more methodical with actually reviewing code but honestly? I kind of like the trust in developers and we're able to get code out a lot quicker.