3 ms·

The death of a thousand round trips has an inverse where the reviewer points out everything they can find, and then the author fix just one thing. You might th

The death of a thousand round trips has an inverse where the reviewer points out everything they can find, and then the author fix just one thing. You might think the author is motivated to minimize round trips, but maybe they will pull the reverse ransom note next where they threatens to find a different reviewer or subvert the code review somehow, because "it's an emergency and the patch needs to go in now".

rwmj 2y ago
I get especially annoyed if I have to tell patch submitters to fix the same thing twice. Carefully read everything I wrote!
bityard 2y ago
This totally happened to me all the time in prior jobs with cultures of low quality and lack of ownership. When I review code, I'm always careful to prefix low-importance things with "[suggestion]" or "[nitpick]". These are small things like typos in comments that would not block me from approving the view. So people who know me know that I don't flag things frivolously. However, I would often review someone's code (someone I know and respect, mind you) with 3-5 changes that MUST be made before I would approve it. Because not fixing it would either be a bug, or would make life unnecessarily difficult later on. They would fix ONE of the things and I would have to point out that they didn't address any of the other things. And then they would go back and fix one more, and the cycle continues until they FINALLY get all the things addressed. Only once have I gotten worn down and approved substandard code because I was tired of all the back and forth. In that case, it was a student whose paid internship was almost up and he had to have it deployed before he left. (We never ended up using that project...)
- 5Qn8mNbc2FNCiVV 2y ago
  I 100% would have closed the MR with a note akin to "If you don't want to work you can just say so". That stuff doesn't fly at all in my teams, it's a disservice to everyone else it's insulting.

rwmj 2y ago

I get especially annoyed if I have to tell patch submitters to fix the same thing twice. Carefully read everything I wrote!

bityard 2y ago

This totally happened to me all the time in prior jobs with cultures of low quality and lack of ownership. When I review code, I'm always careful to prefix low-importance things with "[suggestion]" or "[nitpick]". These are small things like typos in comments that would not block me from approving the view. So people who know me know that I don't flag things frivolously. However, I would often review someone's code (someone I know and respect, mind you) with 3-5 changes that MUST be made before I would approve it. Because not fixing it would either be a bug, or would make life unnecessarily difficult later on. They would fix ONE of the things and I would have to point out that they didn't address any of the other things. And then they would go back and fix one more, and the cycle continues until they FINALLY get all the things addressed. Only once have I gotten worn down and approved substandard code because I was tired of all the back and forth. In that case, it was a student whose paid internship was almost up and he had to have it deployed before he left. (We never ended up using that project...)

5Qn8mNbc2FNCiVV 2y ago
I 100% would have closed the MR with a note akin to "If you don't want to work you can just say so". That stuff doesn't fly at all in my teams, it's a disservice to everyone else it's insulting.