Comment by sarchertech

Comment by sarchertech 5 days ago

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..

greenthrow 5 days ago

If you are never getting good suggestions on your PRs that's a bad sign. Any team of more than 2 people should have some ideas sometimes for each other. Either this means everybody's too checked out to put in effort on PRs or they think it'll fall on deaf ears.

I've been a software engineer for decades and even so, teammates will have good ideas sometimes. Nobody can think of every good idea every time.

Reply View 14 replies

sarchertech 5 days ago

I said very rarely not never. I classify suggested changes in 2 flavors. The first is minor changes where someone suggests "hey this would be easier to read if you used syntax A vs syntax B here".
I get those frequently, and they are usually reasonable suggestions, and I usually graciously accept them. But I say it's not worth bringing up in a PR because 99% of the time it doesn't actually matter in the long run. Both forms will have been used in the code base previously, and which one you think is better really comes down to which one you use more. It's the kind of thing you could change with a sufficiently opinionated linter. And the kind of thing that isn't worth the cost of mandatory PR reviews.
The 2nd is where someone suggests changes to overall architecture or changes to the way the feature or code works large scale.
These are much rarer because of several reasons
1. PR reviews are almost always surface level and so are more likely to catch category 1 than category 2. The incentives at nearly every tech company push for this. 2. Very frequently one person isn't available to review all of the PRs that go into a feature, so the reviewers lack context. 3. It's very unlikely that even if someone wants to dig deeper, that they have the free time to spend even 1% of the time the person who wrote the PR spent on it.
But the biggest reason I personally don't get many of the 2nd category is because I talk through non-trivial problems with other experienced engineers before I get to the PR stage.

Reply View | 4 replies
- greenthrow 5 days ago
  
  You're missing a third category which is "here's another way you could do this..." which isn't just about legibility but more tangible tradeoffs.
  Certainly I agree architectural decisions shouldn't be made in every PR and ideally should be discussed before that point.
  
  Reply View | 3 replies
  
  sarchertech 5 days ago
  
  I'd consider that to fall under my category 2
  "changes to overall architecture or changes to the way the feature or code works large scale"
  I'm not saying there's never a reason to go back and redesign something after a PR review, but in my mind getting a design to that point and then actually needing to change it is a huge failure.
  Far more common the case where someone wants a big design change with no tangible benefits just different tradeoffs.
  I just don't think the ocassional benefit is worth the cost of the process.
  
  Reply View | 2 replies
rr808 5 days ago

The problem is the good ideas should come before or while you start coding. By the time the PR is done isn't the right time to rewrite it.

Reply View | 6 replies
- greenthrow 5 days ago
  
  What? This is absolutely incorrect. There's be no point in PRs if it's too late to change anything. Part of the point is to reduce the number of synchronous discussions your team has to have about code before writing it. PRs let you iterate on actual code instead of endlessly discussing hypothetical implementations.
  
  Reply View | 5 replies
  
  agentultra 5 days ago
  
  This is a huge pet peeve for me.
  You’d kill your teams velocity if you did this for every PR.
  I worked on a team once with… Perfectionist Petra. Petra would jump on an 1200 line refactor that was blocking the team and demand changes that would require it to be rewritten. Not changes that would save the code base from grievous error: Petra didn’t like the pattern used or didn’t approve of the design.
  Sufficient tests? Check. Linted and formatted? Check. Does Petra approve? Big variable. I often wanted to tell Petra if they could just write the code out for me in a ticket so I could copy it for them. Instead I had to try and read Petra’s mind or hope they wouldn’t jump on my PR.
  Sometimes you have to trust your teammate and not let the perfect plan interfere with a good enough one.
  
  Reply View | 4 replies
seb1204 5 days ago

OPP mentioned that this is happening but in discussion prior to PR. Makes sense to me.

Reply View | 1 reply
- greenthrow 5 days ago
  
  I addressed that in the comment you are responding to...
  
  Reply View | 0 replies

000ooo000 5 days ago

>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..

It has value particularly when the team has newcomers or less-experienced developers. Writing it off as 'CYA' discards an opportunity for async feedback to be given on work.

Reply View 6 replies

sarchertech 5 days ago

Sure that’s fine when you’re the one doing the review for a newcomer or less experienced dev.
However, I think that PR reviews after the code has been written are the absolute worst time to do this.

Reply View | 5 replies
- hakunin 5 days ago
  
  I completely disagree. By writing the code, the author gains understanding of the hidden details of the problem. The reviewer gets to explore a full solution that accounts for those details, and provides tests. With the context still fresh, it's a great time for a new pair of eyes to explore, ask questions (which should lead to clarifications in the code), and think of ways to simplify and untangle the solution further while keeping the tests passing. In fact, it can take more time to keep assuming and discussing what the solution might be than to write the code and iterate on it. In an asynchronous/remote work environment, iterating on PRs is not only okay, but probably the most effective way to collaborate.
  
  Reply View | 4 replies
  
  sarchertech 5 days ago
  
  I'm not saying I never fire up a draft PR so that I can nail some things down and get comments on it.
  But at the same time not everything needs to be asynchronous and if you try to make everything asynchronous, such that you never have ad hoc meetings or synchronous conversations, you're going to move much slower than is necessary. I've been exclusively remote for 10 years now, so I love doing thing async when I can, but I recognize the limits.
  By the time my PR is ready to merge the assumption should be that it's ready to merge, unless you see something actually wrong. Not just hey this could be a little easier to reason about if you completely redesigned your solution.
  9/10 when people do make those kinds of suggestions it's not objectively better, and 9/10 changing it won't move the needle with respect to maintainability or meaningful performance.
  So given that we have a funnel of reviewers where most people are only going to do surface level reviews, of those that do deep dives most of them aren't going to find any problems, of those that do most of the solutions aren't going to make any kind of significant impact. Is it worth going through this whole process? My conclusion is that no it is not.
  Comparing the software we produced before the whole PR review for every merge request thing became common, and the software we produce now, I don't think software quality has improved at all. So for me to change my mind, I'm going to need to see hard data that says the opposite.
  
  Reply View | 3 replies

ericyd 5 days ago

This isn't a bad reply but it's fairly off topic in my opinion. I think the majority of reviews on my PR are effectively rubber stamps, but sometimes it takes many business days to get that rubber stamp unless I bug people.

Reply View 1 reply

sarchertech 5 days ago

My assumption was that you didn’t have a group of people who would rubber stamp your PR based on the frustration with the wait time.
If you do have people who trust you, a slack DM when the PR goes up isn’t bugging them. Maybe they just don’t pay much attention to GH notifications.

Reply View | 0 replies

psd1 5 days ago

It must be nice working on a codebase sufficiently well-organised to not explode many files distant to an innocuous change.

Reply View 1 reply

sarchertech 5 days ago

I didn't say I've never pushed a major bug to production, I've just never had one stopped by a PR review.

Reply View | 0 replies