4 ms·

A couple from my pet peeve list - 1) ATOMIC COMMITS Nothing worse than a single commit, to address 10 separate comments. I don't want to have to go back, and

A couple from my pet peeve list - 1) ATOMIC COMMITS Nothing worse than a single commit, to address 10 separate comments. I don't want to have to go back, and cross reference my comments with your wall of changes 2) DO NOT SELF RESOLVE A COMMENT Wait for the reviewer to checkout your solution, I want to learn through your fix, aswell as make sure it's what I expected.

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 ago
  No 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.
colonwqbang 6y ago
Do you make an additional commit for each review comment? As opposed to just amending the old commit. Why?
- nullandvoid 6y ago
  You can squash into a single commit at the end Creating a fresh commit for each comment, provides me an easily clickable diff link
detaro 6y ago
tbh sounds to me as if you review way to large changes if there's 10 comments requesting changes that are not clearly attached to a small code location (and thus can be reviewed through a line/section specific comment, and thus easy to see the individual change)
- nullandvoid 6y ago
  I do agree but some features are complex, and invesitbely PRs will sometimes be large. Using this tactic it help keep things in check even then. I just want to be able to open up a review, click a commit ID to view a diff, and be able to move on Any additional steps are unnecessary friction I feel

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 ago
No 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.

colonwqbang 6y ago

Do you make an additional commit for each review comment? As opposed to just amending the old commit. Why?

nullandvoid 6y ago
You can squash into a single commit at the end Creating a fresh commit for each comment, provides me an easily clickable diff link

detaro 6y ago

tbh sounds to me as if you review way to large changes if there's 10 comments requesting changes that are not clearly attached to a small code location (and thus can be reviewed through a line/section specific comment, and thus easy to see the individual change)

nullandvoid 6y ago
I do agree but some features are complex, and invesitbely PRs will sometimes be large. Using this tactic it help keep things in check even then. I just want to be able to open up a review, click a commit ID to view a diff, and be able to move on Any additional steps are unnecessary friction I feel