review: encourage nit-and-approve #2
Conversation
Summary: When a reviewer approves a change while simultaneously leaving small comments, the change author merges the change after addressing those comments, without needing to wait for explicit re-approval from the reviewer. We call this practice "nit-and-approve". Used appropriately, it saves time and streamlines the review process. wchargin-branch: nit-and-approve
culture/review_culture.md
Outdated
| @@ -140,3 +140,20 @@ doing this, you should ask: in what ways is doing it my way materially better? | |||
| What benefits come from the changed approach? If you don't have clear answers | |||
| to these questions, don't ask for the change. | |||
|
|
|||
| ## Bias toward "nit-and-approve" | |||
There was a problem hiding this comment.
I wonder if we should nuance this encouragement. Perhaps:
## Consider using "nit-and-approve"
With a note that it's advanced level, and the author "when in doubt" should caution with a slower check-in with reviewers before merging.
In my mind "nit-and-approve" is great when the trust you mention is there, and all parties are deeply familiar with review culture. It also takes some tolerance, should the author decide to not include some nits or use a slightly different approach. So I would say it's an advanced level review skill.
Efficiency wise, I believe it would be good to encourage using it. Culture wise, I believe we should be mindful that people who still need to get accustomed to review culture could feel side-stepped.
There was a problem hiding this comment.
Thanks for the suggestion. Done.
culture/review_culture.md
Outdated
| If your review comments primarily suggest local, targeted changes to the | ||
| proposed change, rather than large structural or directional changes, consider | ||
| approving the change on your first review at the same time as leaving your | ||
| comments. This implies that the author of the change should feel free to merge | ||
| the change once they've addressed your concerns. This is especially useful when |
There was a problem hiding this comment.
Non-blocking ask:
I feel like this part is a little dense to read. An easier explanation would help accessibility.
There was a problem hiding this comment.
Okay: I’ve spent some time trying to make the language simpler.
| resolved, and that the author of each comment thread would not be surprised by | ||
| how the feedback was received and incorporated. As a change author, if you | ||
| prefer not to incorporate some suggestion that wasn't intended to be optional, | ||
| you should probably continue the discussion rather than merging immediately. |
There was a problem hiding this comment.
In terms of author do's: I would strongly encourage to comment about how you've responded to (or left out) the nits.
And think we should touch on that. Either in this section, or as a new section and refer to it. Because it's especially important for "nit-and-approve".
Could be very small, like for a typo just fixed! will do.
If it's a larger change, something like this can work:
Thanks, I've taken a slight variation of your suggestion and merged
Or when skipping: This resolves itself in another PR or I think it's OK as-is, leaving unchanged.
When your resolution requires any more detail than that, it's probably better to wait for the reviewer 😃
I think this helps to:
- Recognize the reviewer
- Make sure you haven't missed anything
- Add intuition when you should wait for the reviewer (when the comment becomes a spotty justification)
- Add more context for the public archive
There was a problem hiding this comment.
I agree that it's a best practice to respond to all the nits (whether addressing or leaving unchanged).
There was a problem hiding this comment.
Okay; updated the text.
There was a problem hiding this comment.
Looks good to me as-is. I think nit-and-approve is pretty safe in general, because we already trust gate on having rights to merge. So trusted contributors will who receive the nit-and-approve will be able to merge after addressing the nits; less trusted contributors can receive the warm glow of approval, but their response to the nits will still be (explicitly or implicitly) reviewed by the maintainer who pushes the merge button.
| resolved, and that the author of each comment thread would not be surprised by | ||
| how the feedback was received and incorporated. As a change author, if you | ||
| prefer not to incorporate some suggestion that wasn't intended to be optional, | ||
| you should probably continue the discussion rather than merging immediately. |
There was a problem hiding this comment.
I agree that it's a best practice to respond to all the nits (whether addressing or leaving unchanged).
wchargin-branch: nit-and-approve
wchargin-branch: nit-and-approve
wchargin-branch: nit-and-approve
Summary: When a reviewer approves a change while simultaneously leaving small comments, the change author can merge the change after addressing those comments, without needing to wait for explicit re-approval from the reviewer. We call this practice "nit-and-approve". Used appropriately, it saves time and streamlines the review process. wchargin-branch: nit-and-approve
Summary:
When a reviewer approves a change while simultaneously leaving small
comments, the change author can merge the change after addressing those
comments, without needing to wait for explicit re-approval from the
reviewer. We call this practice "nit-and-approve". Used appropriately,
it saves time and streamlines the review process.
wchargin-branch: nit-and-approve