The 10 Pull Request Commandments
#1 Thou shall not deny a pull request because thou tested the feature locally
Failing someone’s pull request because you tested the code in your local instance is a waste of time. Your local environment may have unknowns in it that prevented the code from working properly. You still don’t know how this code works against the dev branch. If the code isn’t working due to a missing file or misnamed function, leaving a comment saying so would have been much faster than pulling down the code into your local environment.
You might have the best of intentions by checking the pull request’s code locally and it seems like a good idea but it’s not. Code review is just that, “c-o-d-e- r-e-v-i-e-w”. This is not the time for UAT, BDD or Unit Testing. That happens before or after the code review process. While doing code review, you should be checking that your team’s coding standards are being applied properly, the code logic makes sense and that the correct variables, methods or classes are being used effectively.
Code review is just one step out of a much bigger software development life cycle process. There are plenty of tools out there to analyze code, but only humans can catch things that programs are still not able to resolve well. Additionally, you may not even be using these tools in the first place. To address the elephant in the room, YES, this means that you might approve and merge a pull request that possess breaking code even though it satisfies your team’s coding standards.
#2 Thou shall not be punished for updating a pull request with more code that hasn’t been approved or merged yet
A pull request may not have been approved or merged yet because the reviewer left a comment and is waiting on the reply or they haven’t started working on it yet. Ideally, you should have all of the proper code in place before initially making the pull request, but if that’s not the case it’s not the end of the world. In this scenario you can push more code before it becomes a bigger inconvenience.
#3 Thou shall not make changes to the same pull request after it has already been merged or approved
Once PR has been merged, people may not even notice that it has been updated and your new code may not ever get reviewed. It may get stuck in code review limbo. If you make a change to the PR after the PR has been approved it just looks bad for everyone. You might have done so because you forgot to include some extra code that is needed for everything to work properly. This looks bad for the reviewer because they didn’t spot whatever you forgot to include. Continuing to do this hurts the trust relationship you want to establish with your PR reviewer.
#4 Thou shall police the Pull Request Police
A pull request reviewer’s comments doesn’t have to be final law. People are still human with opinions and are prone to errors. Sometimes a comment is just a question that needs to be answered and not a statement which behooves the code to be rewritten. It’s up to whoever submitted the PR to determine if the reviewer’s comments are legit.
A reviewer may point out something trivial like ”remove the extra indention before the function call”,but that may be due to the team’s coding standards. It’s important that you use your best judgment and leave comments for the reviewer in a timely manner to resolve these types of issues. Don’t submit laziness and refuse to make small changes to your PR’s code due to a comment a reviewer left for you. The pros outweigh the cons for doing so.
#5 Thou shall not make a habit out of approving their own pull requests
Certain jobs demand that a different person than has to review your pull request for compliance reasons. Approving your own pull request totally defeats the purpose of code review. Developers have their own biases towards their work and it’s good to get a second pair of eyes to review your changes. This is leads to my next commandment.
#6 Thou shall recognize that Junior Devs Matter
Don’t be afraid to let a more junior developer review your pull request. This is a win-win scenario. For the Junior developer they get to see examples of what strong code from a more senior developer should look like. In a perfect world this Senior Dev’s code should exhibit good programming habits. If the code they are reviewing is constantly solid they will eventually start to pick up the programming habits of the stronger developer, which will please the Senior Developer when it’s their turn to review the Junior Dev’s code. Additionally, the Senior. Dev make gain an opportunity to learn something new and innovative from the Junior. Dev. I’m using Junior and Senior very relatively here. There is always someone better than you.
#7 Thou shall not merge feature requests into master
For small projects, demo or projects where you are the sole developer, it may be ok to break this rule. You should only push to the master branch when you are ready to tag your code, produce release notes and deploy your code to production. Pre-maturely pushing your code to the master branch can also lead to unnecessary git conflicts.
A “dev” or “develop” branch (or whatever naming convention you have agreed on) is the branch that is a combination of all of the feature branches which developers deem stable enough to integrate with the team’s code. The feature branches that have been merged into this branch should have been pulled the latest code from the dev branch and tested it prior to attempting to merge everything back into the dev branch.
It’s important to pull the latest code from dev before merging into it, because other developers may have pushed changes just seconds before you decided to push your changes. If you try to merge your feature branch into the master branch, other developers won’t have to opportunity to test their feature branch against your latest changes.
You will have have to deal with git conflicts when you try to eventually merge the dev branch into master; because there are commits in the master branch that don’t exist in the dev branch yet. There are ways to resolve this but that is out of scope for this article. Save yourself the headache and don’t commit to master if you are working in a team or a project with some complexity to it.
#8 Thou shall not use the same person to review their pull request all of the time
One great thing about doing code review is getting a fresh pair of eyes. This can also mean switching up the pair of eyes that review your code. It’s good to get new input and please always remember that iron sharpens iron. What is good for the wolf is good for the pack. I’m running out of metaphors to use here, but I hope you get the gist of it.
By switching up your peer reviewers you are actually making the team stronger at peer reviewing. Switching up members help other members improve their code review skills, help your get a new pair of fresh eyes on your work, reduces biases within the team and prevents one person from getting all of the PR requests. Sharing is caring, spread the PR love.
#9 That shall not deny a PR due to code which hasn’t been modified in the PR
You might find some code in the file that needs changing, but isn’t related to the task at hand. In this case, it’s best to make a new issue in whatever project management tool you are using. Don’t deny the PR request because of code that exist around the code that was changed.
#10 That shall not let PRs become stale
It’s important that PR request happen relatively fast (same day, next day) in respect to the velocity of the SDLC. If you ignore someone’s PR they may take it personally or feel like they need to change reviewers for progress to be made. The PR may have a dependency on another feature so if that PR doesn’t get to a point where it is approve or denied it can end up becoming a blocker for another PR. Blockers of any kind are not good things to deal with. Also, you want to get stable features into the master branch as soon as possible. Allowing PR requests to get stale prevents this and causes the need to doing more branch updates.