Better Pull Requests
Our code eventually has to work with other people’s code. While the way we used to do this was much more chaotic (just checking in to the main branch and hoping you didn’t screw up). This might have been acceptable in the old days with once-yearly deployments, but it simply will not withstand an environment with more frequent deployments. Additionally, over the years we’ve learned a lot about how to make cleaner, more maintainable code that can be more quickly understood by other developers. However, in order to do this well, one of the best things you can do is to make sure that code meets a standard of quality before it is committed into the codebase. While you could just write a coding standard and try to enforce it, that sort of top down approach is suboptimal. Instead, you’ll find that things work better when the entire team agrees on and protects the coding standards.
PRs (or Pull Requests) are a great way to do this. Essentially, what you do is work on a branch until you have a logical unit of work done. Then you push your branch up and start a pull request into another branch (either your feature branch or master). When the pull request is ready, one or more of your teammates will review the code and make comments on it (or approve it if it is “perfect). If there are comments, you respond to them, either by explaining why you did something a certain way or by fixing your code, and pushing back up. Since your code stays on the same branch, pushing updates should automatically update the PR so that the rest of the team can see the updates. Once the PR is approved, it should be merged into the target branch.
PRs can be nerve wracking, especially if you are early in your career or working with an unfamiliar team. However, it’s absolutely necessary if you want to have a clean, maintainable codebase. Therefore, you need to follow some simple rules to make it more likely that people not only review your code, but that the process will be as painless as possible. While making a proper PR has a lot of technical considerations, it’s also very much a social issue. And like most social issues, there is a lot to be said for considering how other people will react and feel when you do something. As a result, you’ll be far better off with a consistent PR process that is built around making things as easy as possible for whomever is reviewing your code.
Keep PRs small.
If you want PRs to turn around quickly, limiting the amount of stuff you change is a great way to do that. People are more likely to put off reviewing larger PRs. Remember that the difficulty of evaluating code scales at least quadratically with the number of separate things changed. It quickly gets unmanageable when you change a large number of files in a single PR.
Smaller PRs also help with testing scope, especially if your QA writes tests on their own. Smaller PRs make it easier for them to reason about what tests they should write. Small PRs also mean that another developer who runs into merge conflicts with your code will have fewer issues to sort out.
Pre-review your code before asking others to review it.
You should look over your code to make sure that you’ve committed everything you think you have. It’s easy to accidentally forget to add files to source control. Additionally, this is also a good review of the changes right before you (potentially) engage in conversations about the code. This is especially helpful if you’ve been working on your code in between meetings and other interruptions. It helps refresh your memory.
This is also a good time to check for things like spelling errors, code standards violations, and the like. While you might check before you push an update, something about looking at the code in a different environment (for instance, in Azure Devops) makes mistakes more obvious. Pre-checking code to make sure that your code meets standards also helps your reputation with the rest of the team. This can make people more likely to look over your PRs more quickly, because they know it won’t be much work.
Write useful PR header comments and commit messages
Don’t ever write a commit message or PR comment that says “fixed it” or something similarly non-descript unless you are absolutely sure the rest of the team understands what “it” is. When someone goes to review a PR, the comments and commit messages express intent. Part of their job as a PR reviewer is to make sure the code actually does what you say it does. Which means that you have to say something that is provable.
While you should also limit how many separate commits are in a given PR, sometimes you just can’t avoid having multiple commits. Each commit should clearly describe what you are trying to fix in that commit. If your team has more developed processes, it may also be worth having a more involved checklist when PRs are worked, to indicate that things like unit tests and documentation have also been completed.
Focus on a testable (automatic or otherwise) chunk of functionality.
A PR that exposes nothing externally is not particularly useful, except in the very early phases of a new project. If nothing is exposed for testing purposes (and the intent is not to simply look for regressions), then you probably don’t have enough for a PR. The idea here is that someone can take the completed (and merged) PR and test that the code actually works, as opposed to just compiling.
It goes without saying that you generally should not push a PR up if the code can’t compile/transpile etc. Broken code should be an immediate cause for failure of a PR. You may also want to include a validation build to make sure that compilation, transpilation, unit tests, and automation code standards enforcement go through before anyone looks at the PR. This will keep your team from wasting their time looking at a PR that won’t work.
Mind your diffs.
Most of the time, during PR review, people will be looking at a before and after view of your code. Most tools for doing this will highlight changes to the code, whether they are differences, additions, or removals of code. As a result, if you do something that interferes with this, you are going to make your PR harder to deal with. This can include things like changing file encodings, major changes to whitespace, or changes to binaries, all of which can be painful to deal with in some tools.
You should also probably adjust your coding standards so that diffs include as few changes as possible. In practice, this is best done with team wide standards on things like whitespace, brace placement and the like. You should also consider always keeping elements within a file in a predictable order.
Make sure tests pass before submitting.
Your unit tests should pass on your machine before you ever create a PR. Broken unit tests mean that your code is not complete. In general, the same is true of missing unit tests for new work.
Your unit tests should also be able to run in your build (non-development environment). Basically the idea here is that if the code can’t be deployed with a reasonable expectation that it will work, then the PR is not ready. It goes without saying that you should also add unit tests if you don’t have any.
Engage with commenters.
Once someone reviews your PR, you should be responsive when engaging with them. This includes responding to comments in a reasonable amount of time. The important point here is that you should respect the time of your teammates. If someone reviews your PR, has questions, and then waits hours for you to get back to them, it’s very disruptive to their work and probably yours as well.
You should also be careful not to be defensive about any comments that are left. Remember, you are asking for your help to make code better – act in a way that makes such help more likely in the future.
Tricks of the Trade
Pull Requests are a form of communication. Some of the principles here can be applied to other ways to interact with coworkers and people in your life. For example, when you are talking with others keep your stories about yourself small and engage with the other people. Communication is an interaction. Make sure your tests pass, fact check yourself but realize that nobody likes the “well, actually” person. Learning when to speak up and when to remain silent is a life long lesson few ever get good at.