3 ms·

Code review starts out with good intentions. But some gatekeeper inevitably sets up shop and starts rejecting everything for trivial reasons. Says they're inter

Code review starts out with good intentions. But some gatekeeper inevitably sets up shop and starts rejecting everything for trivial reasons. Says they're interested in keeping up 'code quality'. But nothing is worse than keeping buggy code around long after the fix is ready, or delaying features so nobody can try them. I'd recommend a process where comments are allowed but reviewers cannot block commits. Each developer is trusted to be careful, make such changes as are pertinent to the task. Use CI as well and depending on the team it can all go very well.

black_puppydog 3y ago
this works well when coverage/test quality is high. which is in itself not something that magically happens if you just let devs move as quickly as whatever manager thinks is necessary right now.
willseth 3y ago
Then an engineering leader needs to stop that person. Dysfunction can manifest in many ways, and overzealous reviewing is one of them. Changing the process so people can ignore pathological reviewers is a half measure at best. I’m torn about blocking. I do get the sense that it’s frustrating to get that big red block, so in many cases people ask for changes without blocking - basically a “soft block”. But I’ve dealt with plenty of cases where a PR is so far off the rails (usually a jr. developer), I think it’s appropriate to send a message.
- Tainnor 3y ago
  For me, it really depends on the other person. There are some devs that I trust to carefully consider my comments (even if they end up disagreeing), so unless something is catastrophically wrong, I will still approve. But there are some other developers who seem content to just routinely (not just sometimes) ignore anything that is not phrased as a command (such as "I think we should test this better"), and I've adopted the habit of not approving such PRs immediately.
qudat 3y ago
I abhor “every code change needs a reviewer” rule. They are a massive distraction and don’t necessarily lead to better code.
- maccard 3y ago
  The rule isn't just for code quality, it's to increase knowledge sharing.
- Tainnor 3y ago
  There are empirical results about code reviews and they contradict your intuition: https://en.m.wikipedia.org/wiki/Code_review https://en.m.wikipedia.org/wiki/Code_review

black_puppydog 3y ago

this works well when coverage/test quality is high. which is in itself not something that magically happens if you just let devs move as quickly as whatever manager thinks is necessary right now.

willseth 3y ago

Then an engineering leader needs to stop that person. Dysfunction can manifest in many ways, and overzealous reviewing is one of them. Changing the process so people can ignore pathological reviewers is a half measure at best. I’m torn about blocking. I do get the sense that it’s frustrating to get that big red block, so in many cases people ask for changes without blocking - basically a “soft block”. But I’ve dealt with plenty of cases where a PR is so far off the rails (usually a jr. developer), I think it’s appropriate to send a message.

Tainnor 3y ago
For me, it really depends on the other person. There are some devs that I trust to carefully consider my comments (even if they end up disagreeing), so unless something is catastrophically wrong, I will still approve. But there are some other developers who seem content to just routinely (not just sometimes) ignore anything that is not phrased as a command (such as "I think we should test this better"), and I've adopted the habit of not approving such PRs immediately.

qudat 3y ago

I abhor “every code change needs a reviewer” rule. They are a massive distraction and don’t necessarily lead to better code.

maccard 3y ago
The rule isn't just for code quality, it's to increase knowledge sharing.
Tainnor 3y ago
There are empirical results about code reviews and they contradict your intuition: https://en.m.wikipedia.org/wiki/Code_review https://en.m.wikipedia.org/wiki/Code_review