Trading Fish The website of Hector Castro

About / Writing / Talks / Feed

Leaving Comments on My Own Pull Requests

For the record, the process of leaving comments on my own pull requests isn’t something I came up with on my own. I adopted it from a previous colleague of mine, Jean Cochrane.

A while ago, Jean was being onboarded onto a team I was responsible for. Part of the onboarding process involved working through a breakable toy exercise, which is a project similar in toolset to the ones we’d work on day-to-day, but different in scope. As part of going through that, I encouraged Jean to take notes on any steps of the exercise that weren’t clear. Jean took that further and annotated each associated pull request with comments containing their in-context notes.

As a reviewer, it was phenomenal to have those prompts front and center. With them, we could immediately begin cutting through any existing ambiguity and work toward a joint understanding of the changes. It was a refreshing experience, and I’ve been trying to reproduce it for all of my pull request reviewers ever since.

The process

Over the years, I’ve refined the process I use before assigning my pull requests for review. In the beginning, it consisted of making sure my changes worked and looked acceptable. That evolved into ensuring all of my commits represented logical changes to the codebase and were as concise as possible. Later, I began placing extra emphasis on clear and reproducible testing instructions.

I still believe all of these pursuits are important, but leaving comments on my own pull requests is the newest addition.

First, I open the pull request (or a draft pull request—if that feature is available to you). Immediately after, I scan through the changes and proactively annotate important lines with comments. The comments aim to direct the reviewer’s attention to areas of the code I think would benefit from direct engagement. Some examples include:

  • Calling attention to a tradeoff I made
  • Elaborating on a not so obvious change in the change set
  • Self-identifying an area where I wasn’t 100% certain about the approach I took
  • Explaining a concept I don’t think my reviewer has been exposed to yet

While a lot of these issues can be covered in the pull request body, I find that associating the details directly with the relevant lines of code is far more inviting to reviewers. Now, instead of trying to guess the exact changes I was uncertain about, or skipping over unfamiliar parts of the change set, the reviewer receives a clear set of prompts with supporting detail from my perspective.

Real-world examples

1. Calling attention to a tradeoff I made

Here, I needed a way to annotate a container image with revision relevant tags and labels. There were several different approaches to choose from, but since this process is happening in GitHub Actions, I settled on the recommended approach by docker/build-push-action.

It also felt important to leave a comment here because if I was reviewing this pull request and I saw the words “crazy max” strung together, it would have immediately triggered my spidey senses. No offense, Max. :laughing:

Comment from vercel/cosmosdb-server/pull/62

See: vercel/cosmosdb-server/pull/62

2. Elaborating on a not so obvious change in the change set

In this case, I upgraded a library in an earlier troubleshooting step. That didn’t end up resolving the issue, but after I did finally resolve it, I decided to keep the library upgrade in so that the dependencies would be up-to-date.

All library upgrades incur some risk. Are we both willing to agree that the risk is worthwhile here?

Comment from PublicMapping/districtbuilder/pull/410

See: PublicMapping/districtbuilder/pull/410

3. Self-identifying an area where I wasn’t 100% certain about the approach I took

Here, I decided to proactively drop Python 3.5 support from an existing library because it was approaching end-of-life. However, when you’re a library maintainer, these types of changes can have a large impact. I wanted to draw attention to the change so that the maintainers could engage with my decision from their perspective.

Comment from stac-utils/pystac/pull/108

See: stac-utils/pystac/pull/108

4. Explaining a concept I don’t think my reviewer has been exposed to yet

I needed a way to supply Docker Hub credentials to a GitHub Actions workflow so that release specific container images could be published. In this case, I wasn’t the repository owner, so I couldn’t set up the credentials myself. I left this comment to provide the repository owners with as much detail as possible to help make credential set up easy.

Comment from vercel/cosmosdb-server/pull/62

See: vercel/cosmosdb-server/pull/62

Code vs. pull request comments

Differentiating between code and pull request-level comments is a question I get asked often when discussing this technique. While it is important to strike a good balance between the two, I find myself encouraging people to worry less about answering this question and focusing more on creating pull request comments in a thoughtful way. If a reviewer reads a comment and thinks it is important enough to persist in the codebase, that’s an easy suggestion and change. Asking reviewers to request the removal of existing code comments is a heavier ask.

That said, here are few loose guidelines for navigating the decision-making process:

  • If your comment carries relevance beyond the lifecycle of a pull request, consider that it may benefit from being a code comment.
  • If you’re making an architecturally significant decision in a pull request, then it probably warrants a separate write-up in an Architecture Decision Record.
  • If you find yourself leaving lots of pull request comments, reflect on whether your pull request is too large, your comments are truly beneficial, or if the code itself would benefit from more clarity.

Special thanks to Jean Cochrane for exposing me to this technique. Also, thanks to both Jean and Dave Konopka for reviewing my writing.