The comments that go into code versus those that go into the pull request description

When you submit a pull request, there are two places you can use to help explain what you are doing and why you are doing it. One is the pull request description, and another is the code you are modifying. And it’s important to understand the difference between them.

The pull request is where you justify why your change should be accepted. In the title, you spell out the problem you are fixing or the feature you are adding.

Add support for polarity reversal

Fix crash when polarity changes

In a large code base, you may need to be a little more specific.

Add support for widget polarity reversal

Fix widget crash when polarity changes twice in a short time

When somebody is chasing down a regression, they are going to be looking over all of the PRs that went into the branch recently, and having a good title will make it easier for them to identify which changes are likely to be a source of the problem.

For example, if somebody is investigating a doodad crash, they may look into “Add support for widget polarity reversal” because their doodad uses widgets, and maybe the problem is caused by a reverse-polarity widget that their doodad isn’t handling. On the other hand, they can pay less attention to the fix for the crash when widget polarity changes because that’s unlikely to be the reason the doodad is crashing. And if their doodad doesn’t use widgets at all, they may just skip over both of them.

If the PR had used the original titles of “Add support for polarity reversal”, without any mention of widgets, then a team investigating a regression in gadgets would have to dig into the PR (because gadgets also have polarity), only to realize that it’s about widget polarity, not gadget polarity.

The description of the PR talks about the source of the problem and how you fixed and validated it. This is point-in-time information where you justify to your reviewer why the change is needed and why your particular implementation of the change is correct. Discuss alternative designs and why they were rejected (e.g. because they were too risky). Show before-and-after screen shots showing that the problem is fixed. Confirm that associated paperwork has been completed, like unit tests. There might be standard paperwork for this, such as a “checkin template”. (It is often the case that the closer a project comes to release, the more stringent the paperwork. For example, late in the product cycle, you may need to demonstrate that the release management team has deemed that the bug meets the bug bar.)

In other words, the PR description is a point in time statement, providing information that is relevant to the code review itself. It is an exercise in persuasive writing: You are trying to convince the approver that your change should be accepted.

Comments in the code are for talking about the code itself. What is the correct way to call this function? Does it have specific prerequisites? This information is durable: It is information that remains useful even after the pull request completes.

Okay, so let’s do an exercise: I’m going to provide some text, and you tell me where it goes. These are all actual comments (suitably redacted) from PRs I have reviewed.

I have checked all calls to the function, and this was the only one that passed the wrong flag.

This goes into the pull request description. It is justifying why your change is correct, and in particular, it’s answering a question that a reviewer is likely to ask: “It’s great that you’re fixing this one caller of the function, but are there other callers that make the same mistake?” Putting this comment in the code itself would be wrong because the claim is valid only at the time the pull request is made. After the pull request, somebody might add a new call to the function that passes the wrong flag, and it is not true that you validated that new caller.

The JSON schema accepted by this function is documented 〈here〉.

This goes into the code. It is explaining how to use the function correctly. This information is important not just at the time you submit the pull request but also for an indefinite period of time in the future. (At least, until you change the function or the schema.)

The Doodad component will take advantage of polarity reversal.

This goes into the pull request description. It is justifying why you need to implement polarity reversal today. If you put this in the code, the future tense suggests that we are still waiting for Doodad. And future changes to the Doodad might cause them to stop relying on polarity reversal; when they do that, they are unlikely to come and update this comment in somebody else’s component. The comment also suggests that if you confirm with the Doodad team that they don’t need polarity reversal any more, it is safe to remove support for polarity reversal, which might not be the case if other components started using the feature as well.

Still, knowing that Doodad is the intended audience for the feature is worth noting for posterity.

// Polarity reversal was initially added for the benefit of
// the Doodad component.

Bonus chatter: Another thing to consider when making code comments is that the code comment needs to make sense even without the PR description. Suppose you are writing a function with the intention of deprecating an older function that it is replacing. Don’t add this comment to the new function:

// When all clients have migrated to the new function, keep this.

This makes no sense to someone who is seeing the comment without having also seen (and remembered) the PR that introduced it. It sounds like the comment is saying, “When X happens, take no action.”

What you should do is put a comment on the old function:

// When all clients have migrated to the new function, delete this function.