I think the entire map was red on Tuesday in large part because of incredibly racists thoughts and posts like these becoming more and more prevalent. The idea that white people should not be a majority and if they are it's due to racism is an extremely disgusting and racist statement.
I recently quit a job, for a large number of reasons, but the one most relevant to this post was the topic of my github numbers. My manager gave me a "needs improvement" review, citing my "bad numbers" and comparing them to several of my teammates. Their numbers, to me, were laughably high.
I thought I was doing pretty good on PR reviews. I do one or two a day. I read the description, sometimes more than once. I read through the practical testing steps. I follow them, sometimes with a lot of setup required. I really make sure it does what it's supposed to do. I read the code, line by line. Sometimes before I do the practical testing, sometimes after. But I read through the code, several times. I don't know how you wouldn't. Functions are calling other functions I haven't gotten to yet. So I read through the code multiple times. I don't just try to understand what it does, but why it does what it does. Is there a better way? Is this going to change or impact something else it should or shouldn't? A good code review can easily take an hour or more, in my humble opinion. Two reviews a day on top of all my bs meetings and I still have my own shit to do. How does my teammate have triple my PR reviews for the quarter? Seems crazy.
One day I'm pair programming with one such teammate. We finish the task, I create a PR, write a description etc. I press the ready for review button and start to say, "see you later bla bla" and suddenly there's an approval on the PR. It's been 5 seconds, tops. It's the other teammate with 10x code ninja numbers on the team. I remark that approval was pretty quick. My teammate forces a nervous sounding laugh. They work together ever day and seem pretty close, huh.
Anyway, after that, every morning after I made coffee and opened my laptop I'd spend 10-15 minutes just going down the list of open PRs on the project and approving them. I didn't look at the code or even bother reading the description. I just approved them all. I also started opening PRs for stupid shit like minor typos in comments that I'm quite sure no one even reads.
My next review my manager said I had made some impressive improvements and gave me a "meets expectations".
I'm looking for a job in a different industry now. I'm not out of software development, I just don't think I can work on another e-commerce webapp company run by MBAs again. I think I'd rather be broke.
I think this comes down to a lack of shared understanding on what a PR approval means, it sounds like you didn't believe it meant the same thing as your team did. There's not one standard meaning of the "rubber stamp". It's a culture thing and teams should discuss and eventually agree on what they think it means. Mature teams will have guides, checklists and documented expectations.
It's moderately likely that behind the scenes someone was complaining to your manager that you were blocking their work and that the "metrics" discussion was just a cover for that.
Usually when I review a PR I am just sanity checking the overall approach, figuring out or asking how they tested it, and making sure there's nothing crazy that will cause pain for everyone else later. Detail correctness I don't consider my problem because there's no economic way I can verify it. I can usually approve in minutes and I don't consider it a waste of time, I did the things the process needed of me.
Unless something irreversible is happening (and it should be reviewer's job to be aware of that), the fix for a bad PR is more PRs.
There are projects where almost the only thing that matters is approving quickly because this will let your co-workers get on with their job, this culture tends to evolve in orgs with lots of related repos where you need 5 MRs and pipelines (that flow on to each other) to deploy the tiniest unit change. It's completely dysfunctional but it happens a lot.
I imagine there are places where reviewers are expected to spend an hour "raising the bar" on every PR but I've never worked at one. I'm also not sure if I'd want to.
Note that there's not one thing or process that makes sense, it's very context dependent with lots of exceptions. For example, if someone from a "far away" team is contributing to a particular repo for the first time I will probably reach out to them to see what they're trying to do and review more carefully because they likely have limited context vs a core contributor.
Well then why does the PR template used at the company require manual validation steps on every PR if it's not expected that reviewers do them? Do you really think 5 seconds was enough time to "sanity check" the PR? Do you think 5 seconds was enough time to even click the code tab? Why are two "reviews" even required if the reviewers are actually expected to simply rubber stamp every single PR that comes through?
Edit: "the fix for a bad PR is more PRs" has got the be the worst take I've seen in years.
The review requirement often comes from compliance, many standards require a control to stop developers getting code into production by themselves without anyone else looking at it. Sometimes it's because an engineering manager read a book that said it was "best practice" or because "that's what we did at $lastjob". Sometimes it's because someone set up the SCM to require it and never looked at it again. Maybe it made sense to someone 5 years ago but it doesn't make sense now, for the teams and processes you have. It's worth asking and finding out!
I agree 5 seconds is not enough for even cursory review, why even bother, unless it's a compliance thing. In a functional team the thing you do is raise this in retro and discuss what everyone expects and wants from reviewers.
If your take is that blindly approving merges with 0 care is actually a good thing and that I "lack understanding" then I simply have to disagree lol. Good luck with that.
Sorry that I was not clear, my bad, I did not mean to imply that you personally "lack understanding" in any way.
What I should have written was that there seemed to be a lack of shared understanding among the team, (i.e, agreement) on the value, meaning and expected process for PR reviews.
I don't think there is any particular level of care that makes sense in all cases for PR reviews, I believe it depends on industry, criticality, the particular repo and who is doing what. I think the most important thing is that everyone is on the same page about what's expected.
