This is a strange concern to try to address with code-review. I don't think most folks actually expect to catch a lot of bugs by staring at diffs. Testing is the right tool for that job.
> I'm way more interested in that than I am whether variable names use camel case or snake case or whatever.
Style concerns definitely shouldn't be the primary focus of a code review. Ensuring some stylistic consistency through the code base is a benefit they can provide, but certainly not the the most major.
> In fact if your tests are complete
How do you know if the tests are complete on someone's work before they merge it? Don't you need to get in there, and well, review their code to see what tests they wrote? Evaluation of test coverage by a peer is one of the more important parts of a good code review. What other process would you use to ensure incoming patches have the test coverage you want them to have?
> we can always refactor the code later if we need to.
Isn't it better to catch code structure issues when they are fresh in everyone's mind rather than taking on technical debt and addressing it down the road when it will require more effort? We've all written code in somewhat silly ways. My experience is that when a peer calls that out before I merge it, it's way easier to fix up-front than a year down the road when I'm in there trying to fix some bug and am wondering if I should just refactor the mess while I'm at it.
Correct. I expect those tests to be developed alongside the code. Part of my 'definition of done' is you have passing tests to prove the implementation is complete.
> Style concerns definitely shouldn't be the primary focus of a code review. Ensuring some stylistic consistency through the code base is a benefit they can provide, but certainly not the the most major.
Agreed. However, I've seen too many toxic scenarios where this is what actually happens.
> How do you know if the tests are complete on someone's work before they merge it?
Everything at the branch point can be tested. Also, since we have tests, we have the ability to regression test and ensure everything is still working properly after the merge.
> Isn't it better to catch code structure issues when they are fresh in everyone's mind rather than taking on technical debt and addressing it down the road when it will require more effort?
Yes. My counterargument though is having a complete set of tests that all pass is far more important than concerning yourself with the structure of the code. I understand that's a radical departure from the state of the art but it stems from years and years and years, nay decades, of enduring counterproductive code reviews. I've sat in on several sessions where there have been long, drawn-out discussions on flexibility and extensibility when 9 times out of 10 it simply didn't matter. I've seen so much time wasted on stuff that ultimately didn't matter.
My take away: make sure your tests are reasonably complete and they pass. If so then you're good. Maybe take a quick look at the code, especially if being submitted by a junior, to make sure nothing outrageous is present, otherwise don't worry too much about it.
Also reviewers shouldn't squabble about syntax or formatting, that what we got computers for to do for us using auto formatting tools. Though it does help if a team converges on a similar style for which reviews are great as you quickly learn each others style and cross contaminate them with your own.
PR's become more of a gate to see if you understand the problem and test for the solution opposed to HOW you went about it. Maybe I've had bad experiences but focusing on HOW is really prone to toxicity. I try to avoid that in PRs at all cost and foster a culture of collaboration, shared understanding of the PROBLEM and Respect for the Constraints. I've left places because of extremely toxic PR culture, so I'm probably pretty jaded :p (The culture was so bad I dreaded asking for reviews.)
How did you prove that your solution works and respects the constraints?? How do you protect yourself and others from future changes? Everything else is an implementation detail.
I've worked in a place where code reviews where treated to just a box to have checked off. There was no shared responsibility for the codebase. And most people had a mentality of "if you approve mine, I'll approve yours". Still it was kept as a "safeguard" by management because for them it was also a checkbox they could mark on their process checklist. I'm happy I left, there is only so much culture a single person can change in a team/company.