Re:It Takes 6 Days to Change 1 Line of Code
by willseth on Sunday July 16, 2023 @10:51AM
(#36747635)
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.
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.
Re:It Takes 6 Days to Change 1 Line of Code
by black_puppydog on Sunday July 16, 2023 @10:56AM
(#36747682)
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.
I abhor “every code change needs a reviewer” rule. They are a massive distraction and don’t necessarily lead to better code.
The rule isn't just for code quality, it's to increase knowledge sharing.
There are empirical results about code reviews and they contradict your intuition: https://en.m.wikipedia.org/wiki/Code_review
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.