Why this matters
A pull request asks for one branch to be merged into another. Before the merge, someone reads the change. That reading is a code review. A good review catches bugs before users meet them, and it spreads knowledge: the reviewer learns what changed, and the author learns what a careful reader notices.
This lesson covers the review itself: finding the change, writing comments on exact lines, and giving a verdict. The steps for opening a pull request and the three merge styles are in the GitHub, remotes and pull requests lesson of this module, so they are not repeated here.
Terms used in this lesson
- Diff: a list of lines removed and added between two versions of a file.
- Hunk: one block of changed lines, with a few unchanged lines around it for context.
- Review: a set of comments sent together, with one overall verdict.
- Verdict: the overall answer a reviewer gives: Comment, Approve or Request changes.
- Thread: one comment and the replies under it, attached to a line of code.
- Suggested change: a comment that contains the exact replacement text, which the author can accept with one click.
The idea: many comments, one verdict
Think of a teacher marking an essay. The teacher writes notes in the margin next to particular sentences, then writes one grade at the end. The margin notes are comments. The grade is the verdict.
A GitHub review works the same way. You write comments on lines of code, then submit all of them together with one verdict. The author gets one notification for the whole review, not one for each comment. You can also post a single comment on its own. It appears at once and has no verdict. Reviews are the better habit, because you finish reading before the author sees anything.
Reading the Files changed tab
Open the pull request and select the Files changed tab. It lists every file the branch touches. For each file it shows the lines that differ from the target branch. Removed lines are red with a minus sign. Added lines are green with a plus sign. Unchanged lines near them are shown in grey, so you can see where the change sits.
Under the hood the tab draws a diff. A diff is plain text, and you can produce one yourself with the diff program. Take two versions of a small Python file, one before the change and one after. The file old_average.py has this function and two test calls:
def average(numbers):
total = 0
for n in numbers:
total += n
return total / len(numbers)
print(average([2, 4, 6]))The new version, new_average.py, has one extra line at the bottom: print(average([])). Running diff -u on the two files prints this:
--- old_average.py 2026-11-05 10:00:00.000000000 +0000
+++ new_average.py 2026-11-05 10:00:00.000000000 +0000
@@ -6,3 +6,4 @@
print(average([2, 4, 6]))
+print(average([]))Read it one line at a time:
- The two lines starting with
---and+++name the old file and the new file, then the time each was saved. The times on your machine will differ. @@ -6,3 +6,4 @@is the hunk header. It says the old side shows 3 lines starting at line 6, and the new side shows 4 lines starting at line 6.- A line that starts with a space is unchanged. These are context lines.
- A line that starts with a plus sign was added. Here that is
print(average([])). - A line that starts with a minus sign would have been removed. This diff has none.
The Files changed tab shows the same content with colours and line numbers. Two extra controls help with long pull requests. Each file has a Viewed checkbox, which only you see, so you can track what you have read. A dropdown lets you look at one commit at a time, which is useful when the author added a fix later and you want to see only that fix.
Before you comment, read the pull request description. It tells you what the author meant to change. A diff alone shows what changed, not why.
Worked example 1: a case the author did not test
The author added the second call on purpose, to check the empty list. Run the new file to see what it does:
python3 new_average.py4.0
Traceback (most recent call last):
File ".../new_average.py", line 9, in <module>
print(average([]))
~~~~~~~^^^^
File ".../new_average.py", line 5, in average
return total / len(numbers)
~~~~~~^~~~~~~~~~~~~~
ZeroDivisionError: division by zeroThe directory names in the file paths are shortened here with .... Everything else is the real output.
The first line, 4.0, is the first call working. The traceback is Python's report of an error. It reads from the bottom up for the cause: ZeroDivisionError: division by zero means the code divided by zero. The division is on len(numbers), and an empty list has length 0.
A reviewer who reads the code and asks "what does this do with an empty list?" finds this bug before merging. Here is a comment you could leave on the line return total / len(numbers):
This line divides by
len(numbers). For an empty list that is 0, so the callaverage([])raisesZeroDivisionError. Should an empty list return 0, or should the function raise a clear error? Please add a test that shows which one you chose.
The comment names the line, gives the failing input, asks a real question, and asks for a specific test. The author can act on all of it.
Leaving a comment on one line
Move the mouse over a line number in the Files changed tab. A blue plus sign appears beside it. Click the plus sign to open a comment box. To comment on several lines at once, press the plus sign on the first line and drag down to the last line. The comment then covers the whole range.
When you click the plus sign, the comment box has two choices:
- Start a review keeps the comment pending. A pending comment is visible only to you until you submit the review.
- Add single comment posts the comment right away, with no verdict.
Start a review when you plan to leave more than one comment. Each new comment is added to the same pending review. When you have finished, select Review changes at the top of the Files changed tab. A dialog opens. Here you write an overall comment if you want one, choose a verdict, and press Submit review. All pending comments are published with the review.
The button names above come from the GitHub web page, and GitHub sometimes changes their wording. If a name differs, look for the same action in the same place.
What makes a comment useful
A good comment says three things: what you saw, why it matters, and what you suggest. A comment that only says "bad" gives the author nothing to act on. Compare these:
| Weak comment | Better comment |
|---|---|
| "bad" | "This loop reads the file on every pass. Reading it once before the loop would be faster and easier to follow." |
| "wrong" | "The test expects 0 for an empty list, but this line raises an error. Which behaviour is intended?" |
| "fix this" | "The name x does not say what the value holds. total_price would tell the next reader what it is." |
Ask a question when you are unsure. Many review mistakes are about a misunderstanding, and a question lets the author explain.
Suggested changes
When the fix is small and you know the exact text, use a suggested change. In the comment box, use the suggestion button in the comment toolbar. GitHub inserts a fenced block (a code block wrapped in lines of three backticks) with the word suggestion on its first line. Anything inside the block is the replacement for the line the comment is attached to.
Here is the text you would type into a comment on the line return total / len(numbers):
Returning 0 for an empty list fixes this.
```suggestion
if not numbers:
return 0
return total / len(numbers)
```GitHub shows the proposed change as a small diff inside the comment. The author can press Commit suggestion, and GitHub adds the change as a new commit on the branch. The author does not need to copy and paste anything. Two details matter. The indentation inside the block must match the code it replaces, because the replacement is exact. And the suggestion should cover only the lines the comment is attached to.
Use suggestions for small changes only. For a larger change, describe it in words and let the author decide how to write it.
Replies and resolving conversations
The author answers in the same thread. When you are satisfied with an answer, press Resolve conversation to fold the thread away. A resolved thread stays in the history, so the reasoning is kept.
Sometimes a thread goes out of date. If the author pushes a new commit that changes the line you commented on, GitHub marks the comment Outdated and shows the old code it referred to. Read the new code, decide whether your concern still applies, and reply or resolve the thread.
Some repositories are set to block a merge until every thread is resolved. If that rule is on, the merge stays blocked while a thread is open. That is a setting the repository owner chose, not a fault in the pull request.
Choosing a verdict
The verdict is the last choice you make. Each one sends a different signal to the author and to the merge rules.
| Verdict | What it tells the author | Effect on merging |
|---|---|---|
| Comment | "I have notes. I am not deciding yet." | Neither approves nor blocks. |
| Approve | "I read this and I would be willing to merge it." | Counts toward the approvals the branch requires. |
| Request changes | "Something must change before this merges." | On a branch whose rules require approval, blocks the merge until the same reviewer approves or the review is dismissed. |
Some rules of thumb. Use Comment when you have questions or small suggestions and the change is otherwise sound. Use Request changes when a specific problem must be fixed, such as a crash or a missing test. Approve only when you would be willing to merge the change yourself with the notes you left. Do not approve with an open concern, and do not approve a change you have not run.
Two facts to know. GitHub does not let you approve your own pull request, so an author's review is always a Comment. And a later review from the same person replaces their earlier verdict. If you approve on Monday and request changes on Tuesday, your Tuesday verdict is the one that counts.
Worked example 2: a full pass over one pull request
Here is a complete review, step by step, of a pull request titled "Handle empty list in average". It changes one file and adds one test.
- Read the description. It says the function should return 0 for an empty list.
- Open Files changed. There is one file with four changed lines.
- Read the diff. The new code adds an
if not numbers:check that returns 0. The test callsaverage([])and expects 0. - Check that the code does what the description says. Run the test locally. Here you would also try a case the author did not write, such as a list with one number.
- Find a problem. The check is correct, but the comment text in the code is out of date. It still says "raises an error for empty lists", which no longer happens.
- Leave one comment on that comment line: "The comment above says this raises an error. It now returns 0, so please update the wording."
- Choose Request changes, because the code comment is wrong and would mislead the next reader. Write a short overall comment: "Thanks. One small fix, see the comment on line 2."
- The author pushes a fix. The comment becomes Outdated. You check the new wording, resolve the thread, and choose Approve in a new review.
Notice that the review is short, specific and kind. It does not ask for a rewrite of unrelated code.
Worked example 3: an error a reader can find by thinking about the last item
Here is a second function. Its job is to return the largest number in a list.
def largest(numbers):
best = numbers[0]
for i in range(len(numbers) - 1):
if numbers[i] > best:
best = numbers[i]
return best
print(largest([3, 9, 4]))
print(largest([3, 4, 9]))Run it:
python3 largest.py9
4The first call is right. The second call should print 9, but it prints 4. The loop uses range(len(numbers) - 1), which stops one step before the end of the list. The last number, 9, is never compared. The mistake only shows when the largest number is last, so a test with the largest number first passes.
A good review comment gives the input that fails:
With
[3, 4, 9]this returns 4. The loop usesrange(len(numbers) - 1), so it never looks at the last number. Looping overnumbersdirectly would fix it. Could you add[3, 4, 9]to the tests?
This is the most useful kind of review comment. The reader did not rewrite the code. The reader gave one failing input, a reason, and a test to add.
Reviewing from the terminal with gh
The GitHub command line tool, gh, can read and send reviews without opening the web page. The version used to check these commands is 2.99.0. Here are the commands that matter for review:
gh pr diff 123
gh pr view 123 --comments
gh pr review 123 --comment -b "Could you add a test for the empty list?"
gh pr review 123 --request-changes -b "The loop skips the last number. See the comment on line 3."
gh pr review 123 --approveEach line does one thing. gh pr diff 123 prints the diff of pull request number 123. gh pr view 123 --comments prints the comments already on it. The three gh pr review lines send an overall review, with one verdict and an optional body. A flag is an option written after the command, such as --approve. The -b flag gives the body text.
The gh command can send only an overall review. It cannot attach a comment to one line of code. For line comments, use the Files changed tab.
When you leave out a verdict or give two of them, gh refuses before it contacts GitHub:
gh pr review --approve --request-changes -b "x"need exactly one of --approve, --request-changes, or --comment
Usage: gh pr review [<number> | <url> | <branch>] [flags]
...The ... stands for the list of flags that gh prints after the message. The first line is the part that explains the problem.
A Comment review needs text. Leaving the body empty gives this message:
gh pr review 1 --comment -b ""body cannot be blank for comment review
Usage: gh pr review [<number> | <url> | <branch>] [flags]
...Common mistakes
Pressing Comment when you meant Approve. The author sees "commented" and the merge stays blocked. Open Review changes again and choose Approve.
Leaving the review pending. Comments that you started but did not submit are visible only to you. The author sees nothing. Open Review changes and press Submit review.
Leaving comments with no reason. "Wrong" and "bad" cause a second round of questions. Say what you saw and why it matters.
Approving without running the code. The Files changed tab shows text, not behaviour. The empty-list bug in example 1 is visible only when the function is called with []. Run the code or the tests before you approve.
Approving with an open thread. If the repository requires resolved threads, the merge stays blocked. If it does not, the open thread still means the question is not answered. Resolve it, or say what is still open in your review.
Asking for unrelated changes. A pull request that fixes one bug should not be blocked by a request to rename a file in another folder. Say it is out of scope and suggest a separate issue.
Writing a verdict on your own pull request. GitHub does not let you approve your own pull request. Ask a teammate to review it instead.
Practice
Exercise 1
The hunk header in a diff reads @@ -12,2 +12,5 @@. How many lines does the old side show, how many does the new side show, and what is the net change in line count?
Show the answer
The old side shows 2 lines and the new side shows 5 lines. The net change is 3 more lines in the new file. The header alone does not say how many lines were removed, because the old side includes unchanged context lines as well as removed lines. The body of the hunk shows which is which.
Exercise 2
This function is in a pull request. What error does last([1, 2]) raise, and what comment would you leave?
def last(numbers):
return numbers[len(numbers)]
print(last([1, 2]))Show the answer
Running the file prints IndexError: list index out of range. A list of length 2 has positions 0 and 1, so position 2 does not exist. The comment should name the line and the input, for example: "numbers[len(numbers)] is one position past the end of the list. With [1, 2] this raises IndexError. Should this return the last item, which is numbers[-1] or numbers[len(numbers) - 1]?" Here -1 counts from the end of the list, so numbers[-1] is the last item. Asking the question is correct here, because the intent is clear but the author should confirm it.
Exercise 3
A reviewer has one question about a variable name and no other concerns. Which verdict should they choose: Comment, Approve or Request changes? Explain why.
Show the answer
Comment. The reviewer has a question, not a required fix. Approve would mean they are willing to merge the change as it is, which they have not decided. Request changes would block the merge over a question that may not need a change at all.
Exercise 4
Write the gh command that approves pull request number 45, with no body text.
Show the answer
gh pr review 45 --approveThe --approve flag is the verdict. The number 45 selects the pull request. No -b flag is needed when the review has no text.
Exercise 5
A reviewer writes this comment on a line of code: "This is wrong." Rewrite it so the author can act on it.
Show the answer
A good rewrite names what is wrong, shows an input that fails if one exists, and gives a next step. For example: "This returns the first match, but the description asks for the last one. For the list [a, b, a] this gives the wrong position. Should it search from the end instead?" The rewrite does not blame the author. It describes the code and the difference from the stated goal.
Summary
- A review is a set of comments on lines, sent together with one verdict.
- Read the pull request description first, then the Files changed tab. A diff shows what changed, not why.
- Use Start a review when you have several comments, so they are sent together.
- A suggested change lets the author accept an exact fix with one click, for small changes only.
- Choose Comment for questions, Request changes for required fixes and Approve only when you would merge the change yourself.
- Run the code before you approve. The Files changed tab does not show whether the code works.