Positive Culture, Pull Requests & Code Reviews
At Playground Inc. we value having an open and nurturing environment for everyone. We try to instill this type of culture within all of our teams. Pull requests are usually associated with code review and an approval process for getting some sort of functionality into a project or to fix an outstanding software bug — at Playground, the engineering team sees pulls requests as an opportunity to teach, learn and foster different approaches to different problems we encounter in our daily work. An idea isn’t good or bad, it has different trade-offs.
Anatomy of a Pull Request
A pull request starts with an idea. Providing an explanation within the request is a good way of helping everyone on the team understand why the change is being made. We should be mindful and aware that not everyone on the team has a full understanding of the scope of the change. Providing a history of the problem being solved helps any new team members jumping on to the project get more acquainted with the project and the specific pull request in question. The more detail we provide for others, the more helpful it will be in speeding up the process of reviewing and ultimately approving the request.
To help everyone on the team, pull requests should typically only update one logical piece of the project. Introducing multiple logical changes in a single pull request could make it harder for the team to review and is more likely to introduce unexpected bugs.
Reviewer(s): The person(s) that are tasked with reviewing the bug fix / new functionality. As a reviewer we should attempt to keep the pull request approval list as short as possible. Having lingering pull requests could be hindering work on future functionality. A prompt review of a pull request also helps the reviewer if they have any questions for the author as the work of the author will be fresh in their mind.
Author/Reviewee: The person who created the initial pull request. As the author we should try to test the code to the best of our ability to ensure the reviewer(s) are receiving our best work and to help them do their job in an efficient manner.
Title: Providing a descriptive title (Feature/Bug Fix/Refactor) helps members quickly identify what the pull request is about. Many times pull requests are tied to a ticket in a project management software system (JIRA, waffle, etc.) and these tickets usually have an ID of some sort assigned to them. Including this ID in the pull request title is a great way for identifying which ticket it belongs to. The title may be different from project to project, and the team should identify the best format for the project.
Description: Using the description section of a pull request is a great place to provide the extra details that don’t fit in the title. Explaining why the change is being applied will give team members an overall understanding of the change and will help them understand the issue and in-turn help the process move along more smoothly. Another useful method for helping teammates navigate the pull request is to provide a list of the files being affected and a brief description of what has changed. While the pull request has the changelogs for all files, giving people a quick overview of the files affected is a great way of helping them navigate how the changes are related to each other.
Reviewing a Pull Request
Reviewing a pull request can sometimes be difficult especially if the person reviewing has no context as to what the bug fix is or the what the new functionality does. As a reviewer try to be in the right frame of mind when reviewing a pull request. Giving kind and constructive feedback takes time and energy, focus on yourself first. If you are pressed for time, not feeling well or hungry let the author know that you need to take care of those things first. You will be surprised how much of a difference mindset makes when reviewing a pull request.
Understanding the issue/feature will help you move forward in the approval process — before reviewing code try to understand the requirements for the change. If you do not understand something ask the author for clarification.
As a reviewer we should try not to jump to conclusions — instead assume that the author has done something for a reason. A general rule when encountering code / logic that you do not understand is to put yourself in the shoes of the author. Having empathy when phrasing your questions will go a long way in keeping them engaged and in a positive mindset to help answer any questions you may have. The feedback you receive from the author may surprise you and help give them a better understanding into the logic they used to solve the problem.
A reviewer’s questions should be as important as the pull request itself — as a team we should not be “nitpicking” stylistic code choices. We should be reviewing the logic and structure of a pull request while avoiding the trap reviews about aesthetics of code.
If the pull request passes all tests and is ready to be merged, providing a positive message is a great way to end the pull request process. Let the author know that they did a good job! Using gifs and emojis is a fun way to do just that.
Creating a Pull Request
As mentioned earlier pull requests should only be modifying a single piece of logic. Author(s) should keep changes small so that the reviewer(s) can review the change easily and quickly. Large pull requests can complicate the review process and can lead to some missed testing coverage by the reviewers. Help the reviewers by keeping the pull requests small so they can approve the requests with as little issues as possible. Another great method for the author to support the team is linking to any documentation that may be relevant to the changes whether it is a library being used or documentation written by the author. The author should try to avoid refactoring code in addition to any logical changes. Refactoring should be done in its own pull request to avoid uncaught errors that may occur. It’s great practice to also speak with the reviewers while working on the feature/bug. Getting early feedback from your peers about different implementation options may help relieve any stress that may come with a code review.
Be mindful when receiving feedback, the author should try to assume that the reviewer has the best intent in the feedback they provided. Giving constructive criticism may be the hardest part of a pull request especially if the code doesn’t pass certain tests or if the author has forgotten a specific condition required in the feature. Though we identify different people within the ecosystem of a pull request (author/reviewer), everyone should think of themselves as the owner of the project as everyone is working towards the same goal in creating amazing code!
In Conclusion
When creating/reviewing a pull request it is important to remember that it’s not only about the code. Remember that there people involved on both sides and that we’re all are trying to accomplish a common goal. This practice not only benefits the project but it can help you develop your listening and communication skills as well. This makes you into a stronger teammate who is capable of providing mindful, constructive feedback.
Resources
Here are a few resources that we found to be inspiring for us. They provide examples of technical pitfalls to avoid, communication guidelines to follow and an overall framework to use when creating and reviewing pull requests.
http://amyciavolino.com/assets/MindfulCommunicationInCodeReviews.pdf
https://www.atlassian.com/blog/git/written-unwritten-guide-pull-requests
http://blog.ploeh.dk/2015/01/15/10-tips-for-better-pull-requests/
