Skip to content

Instantly share code, notes, and snippets.

@SitaGomes
Created March 8, 2023 20:06
Show Gist options
  • Save SitaGomes/adbb6d4d805063a65aec0d7e0728d6cf to your computer and use it in GitHub Desktop.
Save SitaGomes/adbb6d4d805063a65aec0d7e0728d6cf to your computer and use it in GitHub Desktop.
How to write a good pull request description
## What?
Explain the changes you’ve made. It doesn’t need to be fancy and you don’t have to get to technical, yet. Just explicit prose on your net change will typically suffice. At a high level, this is where you let the reviewer know the overall effect of the PR. Reference a ticket in your issue tracker if appropriate, but by all means, don’t just reference the ticket. It’s important to explain what the change is and then and only then reference the ticket. It’s a much better experience for the reviewer if they’re able to spend more time reviewing code and less time studying specification that may not even be applicable on the code level.
## Why?
The “why” is sometimes more important than the “what” of a code change, but we tend to put it after the “what” since we aren’t evaluating theory, we’re evaluating tangible code changes.
The “why” tells us what business or engineering goal this change achieves. It’s the reason we get paid as developers. The “why” is a chance to explain both the engineering goal, but also a some business objective that is satisfied or moved along.
Imagine a situation where you are adding a simple environment variable default value. Less than a line of code. Pretty straightforward and short, right? It’s almost self-explanatory. Except it completely changes the nature of the libraries used in your app. Then it’s probably worth a comment.
## How?
Of course, the PR diff will tell most of the story of the “how”, but make sure to draw attention to the significant design decisions. You decided to write a recursive method instead of a loop, pointing out the merits of this will help the reviewer understand your reasoning and in turn provide a better review.
## Testing?
If your team includes applicable tests alongside updates to your code (and you should), merging code without tests or changes that cause existing tests to fail will typically get caught by a teammate or build management tool. You may optionally move the tests to another commit and another PR, but that fans out your liability if things go wrong. Also, you won’t be taking advantage of your CI/CD process if you merge in separate batches, as the first, test-less commit won’t even cause your pipeline do anything different. Worse yet, that first batch may make it to production before a single test is ever run on it in your pipeline.
Some code, especially infrastructure code (say HELM or Kubernetes yaml files) are harder to test. So it’s important to let the reviewer know how you tested them in case you can’t check in tests. Alternatively, you can explain to the reviewer how to test it locally if necessary. Showing the results of tests you’ve run in this section if none are visible in the diff is also very helpful.
## Screenshots
Of course, screenshots are especially helpful for UI-related changes. A simple screenshot of the before and after, or of the current state vs. your local development view, helps the reviewer tremendously. They’re some of my favorite reviews to do.
Even backend code can benefit from a screenshot of the net change. Take for example the result from a CLI tool after you run the code you wrote. Imagine a snapshot of memory layout. The numbers won’t match up when that same code is run on someone else’s box or in a different container, but they still mean something when viewed as a whole. This is a great spot to put a snapshot of that result and help your reviewing teammates out.
For infrastructure code, you may want to either snapshot or copy the result of say a Terraform plan here. If you want to avoid polluting the comment with a huge plan output, use a collapsible section if your version control provider supports it. Remember, this is markdown and you can get creative.
## Anything Else?
You may want to delve into possible architecture changes or technical debt here. Call out challenges, optimizations, etc.
In our running example, let’s say you know of a better way to offload this work altogether.
Example:
"
## What?
I've added support for authentication to implement Key Result 2 of OKR1. It includes model, table, controller and test. For more background, see ticket
#JIRA-123.
## Why?
These changes complete the user login and account creation experience. See #JIRA-123 for more information.
## How?
This includes a migration, model and controller for user authentication. I'm using Devise to do the heavy lifting. I ran Devise migrations and those are included here.
## Testing?
I've added coverage for testing all new methods. I used Faker for a few random user emails and names.
## Screenshots (optional)
0
## Anything Else?
Let's consider using a 3rd party authentication provider for this, to offload MFA and other considerations as they arise and as the privacy landscape evolves. AWS Cognito is a good option, so is Firebase. I'm happy to start researching this path. Let's also consider breaking this out into its own service. We can then re-use it or share the accounts with other apps in the future.
"
(source: https://www.pullrequest.com/)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment