What I don't like in "commercial" code reviews is that most reported issues are there just to fill space. It's busy work. There is this formal comment system. So instead of fixing this typo in a code comment directly reviewer writes a comment. Code style issues that arise, because of absence of a tool like go-fmt also warrant a comment. That's sad.
There are meaningful comments, but usually people have to fill their review quota.
Lowest hanging fruit should be just an in-place edit that would be automatically retested and that a reviewee can easily take as a patch and check himself. Instead of the back and forth bullshit.
It's a bit funny that placing a red herring comment that anyone can safely comment on makes the workflow faster. It's like this story about an artist who would place something ridiculous so when the time comes the CEO would just say: "It's good, just remove the thing".
Part of this process, especially for new team members or junior devs, is to signal that, yes, we take typos (and formatting, and proper grammar, and the style guide, etc) seriously. I could fix this myself, but I shouldn't need to. We all make mistakes, but too many "small" mistakes is an indication of sloppiness. Is this actually a mistake? Or were you just being lazy?
Agreed. In my team I am known for being extremely meticulous about this, and some people don’t like it. If I see one or two typos or spacing issues, I understand that’s an honest mistake. But if I see typos, spacing issues, bad indentation, bad naming, etc. all over your diff, I can’t help but think you’re sloppy. If you don’t care about the little things like properly formatted code, I have a hard time believing you care about the big things like error checking and proper test coverage.
On the other hand, pointless bikeshedding quickly gets mutually tiresome in a setting where every line of code is reviewed in small increments. I also like to address these tiny inconsequential hangups politely when I disagree that they're important. Let's say some guy suggests changing "name" to "someNeedlesslyDescriptiveName" and I'll point out that it's already clear what it is because it's the only variable in the needlesslyDescriptive function.
There are meaningful comments, but usually people have to fill their review quota.
Lowest hanging fruit should be just an in-place edit that would be automatically retested and that a reviewee can easily take as a patch and check himself. Instead of the back and forth bullshit.
It's a bit funny that placing a red herring comment that anyone can safely comment on makes the workflow faster. It's like this story about an artist who would place something ridiculous so when the time comes the CEO would just say: "It's good, just remove the thing".