Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

More etiquette for responding to code reviews #2169

Merged
merged 4 commits into from
Sep 13, 2022
Merged

More etiquette for responding to code reviews #2169

Changes from 2 commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
79cfae1
More etiquette for responding to code reviews
josh11b Sep 12, 2022
f60c2b1
Request review button
josh11b Sep 13, 2022
419613a
Apply suggestions from code review
josh11b Sep 13, 2022
25982bd
Fix formatting
josh11b Sep 13, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
17 changes: 16 additions & 1 deletion docs/project/code_review.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -62,7 +62,7 @@ in line with accepted designs. It may in some rare cases extend to exploring
experimental or prototype directions whose design is under active consideration.

The term "code review" in the Carbon project is not only about "code". We expect
changes to any files to be reviewed, including documentation and any other
changes to any file to be reviewed, including documentation and any other
material stored in the repository.

## Who should review?
Expand Down Expand Up @@ -248,6 +248,21 @@ For non-optional comments, this section provides several suggestions on how best
to make progress. If none of these work, you may need to
[resolve an impasse or conflict](#resolving-an-impasse-or-conflict).

In response to suggestions, update the files in the pull request in new commits.
Rebasing, squashing, or force-pushing commits can break GitHub's comment
associations, and it makes it harder to determine what's changed since the last
review. With regular pushes, GitHub can show individual deltas, giving
additional flexibility to the reviewer. We squash pull requests when we merge
them, so the end result is the same.

It is good to reply to every comment so that the reviewer knows you saw them.
Best practice is to send the reply to all of the comments at once, after the
files in the pull request have been updated. If there are a lot of replies, it
can be helpful to include a message saying whether the pull request is now ready
for re-review, or press the "request review" button to the right of the
reviewer's name:
josh11b marked this conversation as resolved.
Show resolved Hide resolved
![The request review button on GitHub](https://user-images.githubusercontent.com/711534/189789784-fcf18d1b-137b-48ba-959a-0d05cee36c2d.png)
josh11b marked this conversation as resolved.
Show resolved Hide resolved

#### Responding to questions or confusion

Some comments in code review will be questions or confusion as the reviewer
Expand Down