With no customers, one of the purposes of code-review is removed, but it's the lesser one anyway. The primary goal of code-review should _not_ be to "catch mistakes" in a well-functioning engineering team - that's a thing that happens, but mostly your CI handles that. Code-review is about unifying approaches, cross-pollinating strategies and techniques, and helping each other to improve as engineers.
Your attitude towards code-review on the other hand is one I've seen before several times, and I was glad when each of those people were fired.
We did post-merge code reviews. But half our team was on the other side of the planet from the other half (4 people on the team, US, EU. APAC, and AU).
If we waited for reviews before merging, we’d be waiting weeks to merge a single PR. Thus, you wrote your code, opened a PR, did a self-review, then deployed it. We had millions of customers, downtime was a real possibility. So you’d watch metrics and revert if anything looked slightly off.
You would wake up to your PR being reviewed. Sometimes there would be mistakes pointed out, suggestions to improve it, etc. Sometimes it was just a thumbs up emoji.
The point is, there are many ways to skin this cat and to “ream” someone for merging without deploying is incredibly immature and uncreative. You can still review a merged PR.
That process sounds fine to me, especially in a context with either good integration coverage or low downtime cost.
> to “ream” someone for merging without deploying is incredibly immature and uncreative.
I'd agree, but I _highly_ doubt that description was an accurate one. Read through the other comments by the same person and you'll get a picture of their personality pretty quickly.
It's likely that there was already an ongoing conflict either in general or specifically between them about this issue. They probably got a moderately harsh comment to the effect of "hey, you're expected to wait for code-reviews now, knock it off"
I suggested it's possible to write, commit and own code without others' approval to increase productivity and people get _extremely_ defensive about it. It's so odd. It happened in real life and it's happening in this thread now, too. They even attack your character over it.
Yes. Some people get personally attached to code. It’s incredibly frustrating. Some people use reviews to push dogmatic approaches to architecture and/or exert some kind of control over things. Whenever I meet these people in a code review, and they make unnecessary suggestions or whatever, my favorite phrase to say is, “I can get behind that, but I don’t think it’s worth the time to do that right now,” or, “I disagree, can you give an argument grounded in computer science.” With the latter only being used twice in my career, when someone left a shitload of comments suggesting variable name changes, and then again, when someone suggested rewriting something that was O(n) to O(n^2) and claimed it was better and wouldn’t give up.
You want to get the team to a point where you can disagree and commit, no code will ever be perfect and there is no reason spending 3-4 rounds of change requests trying. I think the worst code review I ever had, ended with me saying, “if you’re going to be this nitpicky, why don’t you take the ticket?” (It was extremely complex and hard to read — and there wasn’t any getting around it, lots of math, bit shifting, and other shenanigans. The reviewer kept making suggestions that would result in bugs, and then make more suggestions…)
He came back the next day and approved my PR once he understood the problem I was trying to solve.
Even these days, where I work on a close team IRL, I’ve been known to say, “if there are no objections, I’m merging this unreviewed code.” And then I usually get a thumbs up from the team, or they say something like “oh, I wanted to take a look at that. Give me a few mins I got sidetracked!” And I’ve even heard, “I already reviewed it, I just forgot to push approve!”
Communication is key in a team. Often, if the team is taking a long time to review, give them the benefit of the doubt, but don’t let yourself get blocked by a review.
If the code work/it's tested, review is for sanity checking/looking for obvious bugs.
Anything else is un-needed grooming that's more about the other developer's ego, not about good code (sometimes its to follow some other constraint, but its a good sign the person has a personality issue).
Well, partly that was a mistaken impression because I thought that your comment was also from vrosas. But I think there's enough in there to assess your attitude toward code-review at least a _bit_:
> They have 12 people and no customers, they can almost certainly adopt a very aggressive developer cycle that optimizes almost exclusively for happy-path velocity. You'd never do that at 50+ engineers with customers but for 12 engineers who have no customers? It's fine, in fact it's ideal.
12 engineers churning out code with no code-review at all? That'll produce velocity, for sure. It'll also produce not just an unmaintainable mess, but an interesting experiment, in which you get to find out which of your engineers are socially capable enough to initiate technical communication independently and construct technical rapport _without_ that process helping them to do so. Hope none of them hold strong technical opinions that clash!
Your attitude towards code-review on the other hand is one I've seen before several times, and I was glad when each of those people were fired.