The comments that go into code versus those that go into the pull request description - The Old New Thing

Skip to main content

Dev Blogs

AI

All .NET posts

.NET MAUI<br>ASP.NET Core<br>Blazor<br>Entity Framework

C++<br>C#<br>F#<br>TypeScript

NuGet<br>Servicing<br>.NET Blog in Chinese

Microsoft for Developers<br>Agent Framework<br>Develop from the cloud<br>Xcode<br>ISE Developer<br>TypeScript<br>PowerShell<br>Python<br>Java<br>Java Blog in Chinese<br>Go<br>Microsoft Edge Dev<br>Microsoft 365 Developer<br>Microsoft Entra Identity Developer<br>Microsoft Entra PowerShell

Visual Studio<br>Visual Studio Code<br>Aspire

All things Azure<br>Azure SDK<br>Azure VM Runtime Team<br>Microsoft Azure<br>Azure Cosmos DB<br>Azure DocumentDB<br>Azure Data Studio<br>Azure SQL<br>DevOps<br>DirectX<br>Microsoft Foundry<br>Power Platform

OData<br>Unified Data Model (IDEAs)

Windows Command Line<br>#ifdef Windows<br>Inside MSIX<br>MIDI and music<br>React Native<br>The Old New Thing<br>Windows Developer

Raymond Chen

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