Original post

Over the past few years I authored or reviewed thousands of GitHub pull
requests (PRs), both for work and for personal projects. I’ve come to believe
there’s a small set of useful rules of thumb for what makes a good PR, what
makes a bad PR, and why getting the good ones merged is much easier – both for
the PR author and the reviewer.

Here’s a quick checklist for a good PR. Each item is described in more detail
below.

  1. Make sure the PR is needed
  2. Have an open issue linked in (optional)
  3. Write a useful PR title
  4. Write a detailed PR description
  5. Adhere to the project’s coding standards
  6. Add tests
  7. Make sure all tests pass
  8. Be patient and friendly during code review

Make sure the PR is needed

This is especially important if you’re contributing to a repository you haven’t
worked with much before. Do some research in the existing issues and PRs in the
repository – including closed ones. Is this change already being discussed
somewhere? Was it proposed before and rejected? The code you want to change – is
it there for a good reason?

GitHub offers reasonably good search capabilities in case the project has a
large log of issues in PRs. It’s not perfect, but by running a few searches with
probable keywords there’s a good chance to find something. Another thing I often
do is search the commit history of a project for relevant information (git log
--grep
).

Demonstrating some due diligence goes a long way in showing the repository owner
that you’re a serious contributor.

Have an open issue linked in (optional)

An important tool of modern software development discipline is having an
open issue (or bug, or ticket, or however else it’s called in other
systems) to discuss some problem or some missing feature we
want to address.

An issue is more general than a PR description. An issue describes a problem; a
PR describes a solution to that problem. Some issues require multiple PRs to be
solved, and interlinking all these PRs through the issue is critical for later
attempts at archaeology.

If in doubt – open an issue. Add all the context there. The PR will then
reference the issue with a #<issue number> tag – this is something GitHub
understands and will add a link between the two. A PR can also say Fixes
#<issue number>
if merging this PR means the issue is fully solved.

One of the most frustrating experiences for a repository maintainer is getting
a PR without sufficient context of what it attempts to solve and why
. Having
an open issue with all the details is the best way to establish this context;
the next sections address some additional ways.

I marked this section as (optional) because an issue isn’t necessary in some
cases. For example, typos in comments typically don’t require an issue and a PR
carries sufficient context. Minor changes in documentation also don’t require
issues in most cases.

If in doubt, create an issue. Linking this to the previous section – if an issue
describing the problem already exists, make sure to link your PR to it –
maintainers love PRs that solve open issues.

Write a useful PR title

This advice will read a bit like “what makes a good git commit message”.

The PR title is extremely important. It’s what people see when listing all open
PRs. It’s also commonly translated to be the first line of the merged commit,
and shows up prominently in git log, etc. Take special care in crafting the
PR title to be descriptive and useful, but not too long.

Some large repositories have special guidelines for writing PRs. For example,
the PR title would start with the component name – “storage/remote:
increase widget timeout”
. Look around – how do other PRs (that were
successfully merged) look? Is there any contribution guide in the repository
that details these conventions?

Write a detailed PR description

This ties strongly to the “Have an open issue linked in” advice. If the PR
requires a long background description, it’s better to do this in an issue and
have a link in the PR. If there is no issue for some reason, the burden is on
the PR description to explain the motivation for the change, and the approach
taken in it.

But PRs and issues are also for diferent purposes. Sometimes, a PR description
will have information that doesn’t belong in an issue, such as details of the
specific approach taken in the PR, benchmark numbers for this PR, etc.

The PR description will make it into the git commit log – add as much detail as
you can. The repository mainainer can later tweak the commit log so they will
remove things they don’t need; if in doubt, add more details.

Adhere to the project’s coding standards

Does the project have a contributors guide? Spend a couple of minutes looking
for it.

Look at other PRs that were merged – what did their authors do?

Look at some of the existing code in the repository – try to match the style of
your PR to the prevailing style in the existing code. Doing this shows the
maintainer that you’re a serious contributor who cares about the long term
health of the project.

Be attentive to the smallest details: how much whitespace does the code have,
including in comments? Is there a specific writing style – Oxford commas, one or
two spaces after a period, and so on?

Add tests

If you’re adding new code – make sure it’s tested. Either add new tests, or
point out in the PR description which existing tests cover it. If a test is
too hard to add for some reason, explain why.

For changing existing code the situation is a bit more nuanced. Is the change
addressing a current test failure? Which one? Which tests are affected by the
change? Should new tests be added?

Spend a few minutes thinking about this and documenting your conclusions in the
PR description. Again, this shows the maintainer that you’re a serious
contributor and your PR is more likely to get attention.

Make sure all tests pass

Many GitHub repositories have integration with CI tools, whereupon each PR gets
automatically tested and the CI system adds notes to the PR about its passing
or failing.

After sending a PR, watch out for this and address any failures. Maintainers are
unlikely to pay attention to PRs that break the build or tests.

If the repository has no such tool, make sure to run all the tests you find in
the project and ensure that your change doesn’t affect them negatively. If some
test breaks because it’s bad, make sure to fix it. If some tests fail with or
without your change, make sure to call this out in the PR description.

Be patient and friendly during code review

Once the PR is finally out, the contributor’s task isn’t done yet. In fact, most
of the work may yet be ahead. The code review process is an important tenet of
modern software development, and knowing how to behave is critical for success.

Whole book chapters have been written on code reviews, so I’ll keep it short and
simple here, focusing on open-source contributions. When sending a PR for a work
project, it’s obvious we should be extra friendly and kind with colleagues,
right? Right?

But what about open source? You found an issue in some OSS project you use, and
are sending a PR. Good for you! Do you expect the maintainer will be delighted
about the contribution? Well, not necessarily, and it really depends on your
demeanor.

OSS maintainers are notoriously overworked and underpaid. Sometimes they just
want stability – as few changes as possible. Clearly if you report a critical
bug they will likely be happy to fix it; but 99% of PRs are not for critical
bugs – they are for minor bugs and new features. Here the PR presents a
dilemma for the maintainer – someone is sending code, and this someone is very
likely going to completely disappear after the PR lands, so the responsibility
for the code moves to the maintainer. It’s not surprising that in many cases,
maintainers are cautions and even suspicious of every change.

When answering review comments be patient and friendly. Assume good
intentions – the maintainer is taking extra burden to maintain additional code
(especially with feature PRs) and it’s their right to scrutinize it and take
their time. Multiple rounds of reviews may be required. Be sure to be responsive
and attentive to detail – acknowledge all comments, either by doing what the
reviewer suggests, or (kindly) explaining your point of view. Add more tests if
they ask you to (and you can’t point to existing tests covering the same thing),
add more comments if they ask you to.