Reviews for quality are hard and time consuming. I personally can’t really review the code looking at the diff, I can give only superficial comments. To understand the code, most of the time I need to fetch it locally and to try to implement the change myself in a different way. To make a meaningful suggestion, I need to implement and run it on my machine (and the first two attempts won’t fly). Hence, a proper review for me takes roughly the same time as the implementation itself.
I think this isn’t true for most PRs I deal with. To make a meaningful suggestion, I often need to think about the code by browsing the project’s source code. On projects that I really know, I may have already thought how to implement certain feature and it’s fun to see what others came up with. I only run the code that I’m reviewing when I suspect there’s something wrong with it but it’s not clear from reading the code. I generally don’t try to implement it myself unless I came up with a different solution and explaining it in words takes more time than writing some code / pseudo code.
So, instead of scrutinizing away every last bit of diff’s imperfection, my goal is to promote the contributor to an autonomous maintainer status. This is mostly just a matter of trust. I don’t read every line of code, as I trust the author of the PR to handle ifs and whiles well enough (this is the major time saver). I trust that people address my comments and let them merge their own PRs (bors d+). I trust that people can review other’s code, and share commit access (r+) liberally.
This saves time, but doesn’t improve quality. It’s the objective of code review to find bugs and design issues. IMHO, it’s a waste of time to pretend the code was reviewed. Even the most senior programmer makes mistakes. Scrutinizing code from someone who is senior is as important as scrutinizing code from someone who is not.
I think this isn’t true for most PRs I deal with. To make a meaningful suggestion, I often need to think about the code by browsing the project’s source code. On projects that I really know, I may have already thought how to implement certain feature and it’s fun to see what others came up with. I only run the code that I’m reviewing when I suspect there’s something wrong with it but it’s not clear from reading the code. I generally don’t try to implement it myself unless I came up with a different solution and explaining it in words takes more time than writing some code / pseudo code.
This saves time, but doesn’t improve quality. It’s the objective of code review to find bugs and design issues. IMHO, it’s a waste of time to pretend the code was reviewed. Even the most senior programmer makes mistakes. Scrutinizing code from someone who is senior is as important as scrutinizing code from someone who is not.
This reminds me of Pieter Hintjens’ approach in a good way.
Yup, C4 is probably the document that defined how I see these things early on.