Pull requests (PRs) are a daily part of your work life if you work as a software engineer. Almost all of us have an account on Github, Gitlab or a similar internal tool.

Here is my guide with everything I think a good pull request needs (and how to review pull requests effectively).

Create a pull request

Aim for as small as possible

While some argue that multiple small PRs can waste time when compared to fewer but larger PRs, I believe its benefits outweigh the drawbacks. I can understand why they think this, and I used to feel the same. It feels like it is just easier to do a huge PR with all the features instead of splitting it up. But splitting it up into small PRs has many benefits. And I think it doesn't even save much (if any) time due to the overhead of multiple PR reviews. The exception to this is if your team is extremely slow to do PR reviews...

• small PRs get proper attention. A subtle bug is much more likely to be found in a 10-line PR than a 500-line PR.
• small PRs are also just easier to review. It takes a lot less time and mental effort to review a small PR which is doing one clear thing.
• small PRs are much easier to understand in your head, as a reviewer.
• lastly, I think it's easier to notice in small PRs where there is some feature/flow that isn't tested adequately.

There are some drawbacks to tiny PRs - you could split up a feature into 5 small PRs, each PR looking error-free. But then reviewers are missing the big picture of how it all works together. If you are splitting a feature up into small PRs I would recommend trying to get the same reviewers on all of them as they have the background context about how it will work together.

Self-review before requesting others review your PR

Self-reviewing PRs can significantly reduce bugs and accelerate approval.

My process is always to create a draft PR, let it run through CI checks, and then review it myself. I always use Github, so I use the 'pending comments', and make notes of changes.

E.g. "move this to new file", "change to camel case", "rename unclear variable", "is there a bug when x is 0 here?", "add test for this if condition" etc.

Then I go and fix the issues I just found.

Comment your PR - for others to read

Avoid excessive in-code comments. Use them to explain the 'why' rather than the 'how'. Otherwise, it's more code to read, the comments easily get out of date, and are often redundant.

However, in PRs I love reviewing it when the author comments (on the pull request itself) on their changes as they go.

Try to leave a comment in a place where it doesn't mess up the flow of reading the changes. I will often just comment at the top or bottom of the file, so it doesn't appear in the middle of my changes.

Just explain your thinking process. Maybe it's obvious. But also sometimes it can spark an idea from a reviewer and they can think of an alternative or other improvement.

Give your PRs a good title

Add a clear title/description for your PRs. It is really hard to review PRs when the title is just "fix bug" or "JIRA-123" (some ticket number).

On a similar note: Ideally your git commit should also be descriptive. I'd recommend git committing as often as possible - but before you make a PR ready for review squash up some of those commits so it's a cleaner history and easier to read.

And write a nice description

I tend to work on frontend applications - and for those, it is great when someone includes a (short) video showing the new feature/interaction.

It makes it much easier for the reviewer to understand what is going on in the PR.

But if a video isn't suitable, even a screenshot or a description explaining what is going on is useful.

It can also be good to explain:

• how to reproduce the issue
• a link to an example 'item' (e.g. product page) with the bug/issue
• how to use the new feature
• what kind of setup is required to use the new feature (e.g. "sign up as a user on our free tier to see this new banner")

Seek early feedback on PRs before they're ready for full review

If you are working on something with multiple ways to implement it, then even if you have discussed a plan with your team it is always a good idea to ask someone to skim over the general idea of the change. It is better to catch things early (maybe someone can see a reason why your implementation will not work for some specific case).

Just be clear when seeking 'reviews' that it's still a draft and you want some early feedback. Otherwise, they may wonder why it is in such a bad/incomplete state.

Include tests

I am a fan of test-driven-development (hence the name of this site - code-driven development), where (failing) tests are written first before making a code change. So I am going to suggest that all PRs have tests.

The tests should be clear to read, so reviewers can tell that it's testing the new functionality.

Of course, there are exceptions, maybe some PRs don't need tests...

• If it's a trivial change like a copy/text change, what value do we get out of a test?
• If it's a bug that is affecting customers right now, then sometimes it can be ok to do a hotfix to production without tests. (if the app is broken anyway...). But be careful with this, as it can just introduce more issues. I'd favor doing a rollback if possible than a rushed hotfix but that is another issue.
• And the reality is that sometimes on legacy systems it is so much overhead to do automated tests.

Reviewing a PR effectively

Here are some tips from the other side of the fence - when reviewing a PR.

Look to approve the PR

Most issues brought up in PRs are broadly in these categories:

Issues that must be fixed (do not hit approve):

• real bugs (must fix!)
• security issues (must fix!)

Issues that should be fixed, but maybe can ship:

• edge case bugs (very rare, unlikely to happen)
• not ideal UX (can we ship with the bad UX and then improve in the future? Do we get value with this PR as it is?)

And issues that in the grand scheme of things do not matter:

• styling issues (just use eslint etc)
• things like loops vs map/forEach. It's not the end of the world.

Try to look to approve, but get a nice culture of adding optional comments.

Figure out if the feature works as expected

Ideally, you can easily fire off a 'preview branch' with these changes. This is easy on a lot of FE applications (services like Vercel or Netlify make it easy). Try it out in your browser, and see if it works as you would expect.

If it isn't clear - leave a comment asking what would happen in certain situations.

Check there are adequate tests

It's easy to forget about a useful test when writing code (especially if not following test-driven-development).

Have a look at the tests, are there some useful test cases to add? What happens if undefined is passed? Does it work with 0, or dates in the past?

Be clear about required changes and optional nice to have suggestions

This can vary a lot depending on your culture in the company that you work at. But leaving 'optional' comments are often good and beneficial - as long as they are truly received as optional comments and people know that they can ignore them without pressure.

Opt for direct communication for complex discussions

Sometimes you can get into comments with a dozen or more replies going back and forward with suggestions or trying to understand what is going on in the code.

It is almost always easier to just speak directly and have a real conversation away from GitHub comments