3 ms·
Agree on (2), but I respectfully disagree with (1). It adds a lot of friction for the author to have to make 10 separate commits to address 10 comments. I also
by mtlynch 6y ago
Agree on (2), but I respectfully disagree with (1).
It adds a lot of friction for the author to have to make 10 separate commits to address 10 comments. I also don't want to review 10 separate commits.
The majority of my comments are on separate chunks of code, so it's not that hard for me to see the resolutions to all of them in one big diff.
Edit: I'm assuming the commits will be squashed at the end of the review. If you're preserving the full commit history, then I can understand why you'd want the atomic commits for each note addressed.
- nullandvoid 6y agoNo problem I understand, it took me a while before I ended up sticking with (1). I still think it's much easier for the author to make those changes, and attach a commit ID to the relevant comment, than it is the other way around. As the article states, respect the code reviewers time. If i've put in some effort to point out issues, and you don't even spend the effort to address each comment individually, then you are not respecting the reviewers time. Maybe there are some cases you are right, but as a general rule i'll just always follow (1) as an author, as it honestly doesn't cost me much time, and makes my reviewers life easier.