Pull Requests: merging good practices into your project (Part 2)

<h3 id="disclaimer-">Disclaimer: </h3><p>This article has tons of examples, so I've divided it in 3 parts to make your reading experience more practical. If you haven't yet read the first one, <a href="https://www.vinta.com.br/blog/2018/pull-requests-merging-good-practices-your-project-part-1/">click here to read it.</a></p><p>As I've mentioned before, here I'll be presenting a series of good practices to be put into use when reviewing and creating pull requests. It's not always possible to follow every single one of them and this list should not be taken as a must do, but I'd suggest trying to apply as many as you can. Now let's get into them!</p><h2 id="best-practices">Best Practices</h2><p>Pointing people’s errors is fun and easy, but what really separates an average post from a good one is providing alternatives and solutions to the errors and issues involved. Let’s check some tips and best practices that can greatly improve the code review process as a whole, in its various stages.</p><h3 id="pull-request-templates">Pull Request Templates</h3><p>The human memory is error-prone, people are inherently lazy to look for instructions (hell, most people - including me - won’t even read an instructions manual that’s right in front of them). Creating a template for your repository that contains every information you consider important when creating a Pull Request, along with a checklist that either describes the changes made or reminds the requester to go through some steps solves these two problems with a single blow. The instructions will be right in front of them while they’re filling the PR and they won’t be able to say they forgot to write tests for the feature, because now there’s an app a checkbox for that.</p><figure class="kg-card kg-image-card kg-card-hascaption"><img src="https://vinta-cms.s3.amazonaws.com/media/filer_public/1e/da/1eda6b5c-111c-4bbe-8ff8-ff7c34aaf643/1_pr_template.png" class="kg-image" alt="alt text" title="Pull Request template example"><figcaption><em>Pull Request template example.</em></figcaption></figure><p>Here’s how you can configure those on each of the three main repositories:</p><ul><li>For configuring a template on <a href="https://github.com/blog/2111-issue-and-pull-request-templates">GitHub</a>, you just need to create a PULL_REQUEST_TEMPLATE.md file at your repository’s root or at a .github/ folder in the repo’s root and it’ll be automatically loaded whenever someone tries to create a PR.</li><li>On <a href="https://gitlab.com/help/user/project/description_templates.md">GitLab</a> you gotta create a markdown file inside this path: .gitlab/merge_request_templates/. To make it the default PR template, you must set it on the repo’s settings.</li><li>On Bitbucket, as of the writing of this article, there’s no official way of doing it. <a href="https://bitbucket.org/site/master/issues/11571/custom-pull-request-description-template">There’s an issue open</a> for about 2.5 years now requesting this feature, but it seems that it’s not considered a major issue, so I wouldn’t expect it to be implemented anytime soon. However, there’s a Chrome and Firefox extension called <a href="https://github.com/refined-bitbucket/refined-bitbucket">Refined Bitbucket</a> that allows you to define PR and issue templates, while also providing pretty useful features, like syntax highlighting in PRs, occurrences highlighting, keyboard shortcuts, etc. There’s also a similar extension for GitHub, called <a href="https://github.com/sindresorhus/refined-github">Refined GitHub</a>.</li></ul><p>By the way, at the end of this article you’ll be presented with a checklist and a template example that you can use freely in your projects, so stay tuned!</p><h3 id="repository-configuration">Repository Configuration</h3><p>There are some tiny things you can do in your repository that can make a big difference regarding code management and pull requests. The first one of them is adding status checks to PRs. By configuring external services (Continuous Integration (CI) systems) to run the build with the proposed changes to see if anything breaks, you already save some time from the reviewer, since it makes no sense to review a PR until the build actually works. On GitHub, the steps to configure this are <a href="https://help.github.com/articles/enabling-required-status-checks/">here</a>. On Bitbucket, <a href="https://confluence.atlassian.com/bitbucketserver/checks-for-merging-pull-requests-776640039.html">here</a> (although they don’t mention external services checks). I couldn’t find something like that for GitLab and there’s an <a href="https://gitlab.com/gitlab-org/gitlab-ce/issues/22214">open issue</a> from about 1.5 years regarding it, so I’m not sure if they support it.</p><figure class="kg-card kg-image-card kg-card-hascaption"><img src="https://vinta-cms.s3.amazonaws.com/media/filer_public/af/64/af6480b1-5a30-43b1-9ec7-9bcc2473e2c2/2_0_status_check_bitbucket.png" class="kg-image" alt="alt text" title="Example of status check on Bitbucket."><figcaption><em>Example of status check on Bitbucket.</em></figcaption></figure><figure class="kg-card kg-image-card kg-card-hascaption"><img src="https://vinta-cms.s3.amazonaws.com/media/filer_public/5a/77/5a778a5f-9d26-4057-85cf-c510056089dc/2_status_check_success.png" class="kg-image" alt="alt text" title="Example of successful status check on GitHub."><figcaption><em>Example of successful status check on GitHub.</em></figcaption></figure><figure class="kg-card kg-image-card kg-card-hascaption"><img src="https://vinta-cms.s3.amazonaws.com/media/filer_public/a0/55/a055ab76-629c-42b4-ae8b-507fd8ef2fd2/3_status_check_failure.png" class="kg-image" alt="alt text" title="Example of failed status check on GitHub."><figcaption><em>Example of failed status check on GitHub.</em></figcaption></figure><p>The second configuration I’d recommend is adding guideline files. Much like the templates we saw earlier, these guidelines are markdown files with instructions on how to collaborate with the project (<code>CONTRIBUTING.md</code>) or even files that enforce certain rules in the repo. For instance, <a href="https://help.github.com/articles/about-codeowners/">on GitHub</a> you can create a file called <code>CODEOWNERS.md</code>, in which you define people who are automatically assigned to review pull requests that involve certain areas of the code, files of a certain kind or even specific branches. You can also enforce on GitHub’s settings that merges to a specific branch <a href="https://github.com/blog/2392-introducing-code-owners">can only be made if they have the approval of at least one of the code owners</a> for that branch/piece of code.</p><p>On Bitbucket, you can <a href="https://confluence.atlassian.com/bitbucketserver/add-default-reviewers-to-pull-requests-834221295.html">set default reviewers</a> for specific branches or branch patterns through the repository’s settings, although there doesn’t seem to be a way of enforcing it. There’s an <a href="https://jira.atlassian.com/browse/BSERV-10144">open issue</a> for adding support to CODEOWNERS like on GitHub.</p><p>On GitLab, there’s an <a href="https://gitlab.com/gitlab-org/gitlab-ee/issues/1012">open issue</a> for about 1.5 years now requesting something like those features, but it seems it hasn’t been implemented yet.</p><figure class="kg-card kg-image-card kg-card-hascaption"><img src="https://vinta-cms.s3.amazonaws.com/media/filer_public/4c/9c/4c9c3970-f6cb-4a7f-8bf2-e57a87679580/4_contributing.png" class="kg-image" alt="alt text" title="Example of CONTRIBUTING.md file. Source: Django Project on GitHub."><figcaption><em>Example of <code>CONTRIBUTING.md</code> file. Source: Django Project on GitHub.</em></figcaption></figure><h3 id="gitflow">GitFlow</h3><p>Organizing your repository by using a nice, consistent architecture ensures users can always create pull requests based on the latest stable (or nightly) releases, avoiding duplicated work because it was not clear where the latest changes were. Utilizing <a href="http://nvie.com/posts/a-successful-git-branching-model/">GitFlow</a> and its recommended practices is a great way of achieving this goal.</p><p>This is already included in the GitFlow idea, but it’s never too much to reinforce: it’s usually better to create a separate branch for each feature that’s being developed. It might be tempting to include several minor features in a single branch to be reviewed, but this might complicate the deployment, since one or more features on that branch might be approved, but since one of them isn’t, none of the others can be deployed. Remember, <strong>branches are cheap and bring great flexibility</strong>.</p><h3 id="pr-size">PR Size</h3><p>This might sound picky or like complaints from someone who’s too lazy, but it’s actually a very good practice: try to keep your PR sizes to a minimum. It goes along with what I’ve mentioned before about creating a separate branch for each feature. The more features you put in a branch, the bigger the PR will become. Reviewing PRs can be entertaining and educational, and it definitely saves time in the long run, but it’s still a time-consuming task. We’ve all seen it (and maybe created some ourselves), pull requests with 60+ files in it. Ain’t nobody got time for that!</p><figure class="kg-card kg-image-card kg-card-hascaption"><img src="https://vinta-cms.s3.amazonaws.com/media/filer_public/e9/0e/e90e7def-836f-469d-be84-ee4e9f3c0c58/5_pr_size.png" class="kg-image" alt="alt text" title="Example of a terrible PR with too many files."><figcaption><em>Example of a terrible PR with too many files.</em></figcaption></figure><p>Not only that, but when you have to review a huge PR, the review quality tends to decrease, since you can’t dedicate the same amount of time for each change that you would in a small one. Furthermore, human beings tend to get bored when performing long, tedious tasks, and we don’t want a bug passing by because someone thought it’d be nice to create a 3-digit PR. Remember the KISS principle, it also applies here.</p><h3 id="review-deadlines">Review deadlines</h3><p>Yeah, PR reviews are not the highest moment of some people’s day. Sometimes you’re in a tight schedule with your own features and you just can’t drop it to review someone else’s chances. They might not even be blocked by the review, since they have other features to develop, so why go out of your way to do it?</p><p>Well, in some aspects, PR reviews are much like dirty laundry or dishes in the sink: if you don’t deal with them, they’ll just keep accumulating and eventually you’ll have so many you can’t even handle. Make it a habit of reserving a few minutes of your day to review pull requests. If you and your colleagues follow the previous advice, PRs should be reasonably sized, which hopefully won’t take much time to review. Maybe define one or two days in the week where you have a reserved timeslot for emptying your queue of PRs to review, no matter what.</p><p>When we see a sink that’s already filled with dirty dishes, it’s easy to simply put another one, but when we see it clean and shiny, we’re more inclined to keep it that way. Can we maybe create a KYSS acronym? <em>Keep Your Sink Shiny</em>? No? Okay, moving on…</p><h3 id="clear-commit-messages">Clear commit messages</h3><p>When creating a pull request, often it is good practice (or even required, especially when templates are involved) to add a description of the issue being solved. However, sometimes, due to lack of time or simply because the context is pretty clear in the head of who created the PR, a few details are left out of the description which could be useful to the reviewer.</p><figure class="kg-card kg-image-card kg-card-hascaption"><img src="https://vinta-cms.s3.amazonaws.com/media/filer_public/5d/d9/5dd98c57-7c68-4375-92f9-6eb68fc5c773/6_commit_msg_good.png" class="kg-image" alt="alt text" title="Well described commit message."><figcaption><em>Well described commit message.</em></figcaption></figure><p>For that reason (and also for making it easier to revert commits when looking at the log), make sure to always write clear commit messages when creating them. Avoid naming your commit “fix issues” or “solving problem X”. Try to be a bit more precise on what you did, so people can have an idea of what was the intention there (when reviewing code) and which files were modified (when reverting commits/navigating through the log). We hear so much about User Experience (UX), but maybe we should also give some thought about the <a href="https://hackernoon.com/developer-experience-dx-devs-are-people-too-6590d6577afe">Developer Experience (DX)</a>. Don’t be afraid to use the second paragraph of a commit message. Keep the first line concise, but clear (it’s meant to be a summary), but feel free to use the following paragraphs to explain changes in detail. For further information in this topic, check Git’s <a href="https://git-scm.com/book/en/v2/Distributed-Git-Contributing-to-a-Project">commit guidelines</a>.</p><figure class="kg-card kg-image-card kg-card-hascaption"><img src="https://vinta-cms.s3.amazonaws.com/media/filer_public/ca/78/ca78a864-c8e7-44bf-a41d-3bd1a6d9a1c5/7_commit_msg_bad.png" class="kg-image" alt="alt text" title="Badly described commit message."><figcaption><em>Badly described commit message.</em></figcaption></figure><h3 id="positive-vs-negative-feedback">Positive vs. negative feedback</h3><p>This subject has been mentioned in quite a few articles over the web, but still I sense that not enough people put these ideas into practice (ahem, <a href="http://gizmodo.com/you-dont-need-to-understand-programming-to-appreciate-t-1739927472">Linus Torvalds, we’re looking at you</a>) and there’s still some people who think this is overreacting to something that doesn’t deserve this much attention.</p><figure class="kg-card kg-image-card kg-card-hascaption"><img src="https://vinta-cms.s3.amazonaws.com/media/filer_public/d8/ca/d8ca55af-63a2-4470-a61d-4a444133a5bc/8_linus_feedback.png" class="kg-image" alt="alt text" title="Linus Torvald’s bad example of feedback."><figcaption><em>Linus Torvald’s bad example of feedback. <a href="http://lkml.iu.edu/hypermail/linux/kernel/1510.3/02866.html">Source</a></em></figcaption></figure><p>We spend so much time at work dealing with computers that we sometimes forget <a href="https://medium.com/@real_tadas/theres-a-human-on-the-other-side-of-your-code-review-9732cc15bfee">there’s a human on the other side of your code review</a>. Although you don’t need to (and actually shouldn’t) give super lengthy responses to pull requests, it doesn’t hurt to be nice to whoever’s on the other side. First of all, that person is trying to collaborate (be it because he/she works at the company or simply because he/she felt like contributing out of good will), it’s his/her time and energy being spent to improve the software you maintain, so the least you can do is to be polite.</p><p>Moreover, providing positive feedback (keep in mind that positive feedback doesn’t mean to agree with whatever the person says) helps the communication to flow better, serves as an incentive for the person to perform the changes you’ve suggested in the way you’ve suggested. If you’re rude to that person, any suggestions you make will bring along a heavy load of negativity.</p><p>A positive environment allows for more willingness to collaborate and expose ideas. People will feel more at ease to make suggestions without fear of being judged if it ends up being a silly idea. Many great ideas start as a silly little thought that was thrown in, like a rough stone that’s polished over and over until it becomes the shape, size and color that we’re looking for. By not allowing this environment to exist, we risk never seeing these gems. Also, even if the idea actually is not good, at least the person has the opportunity to get that feedback sooner than later (remember, it’s better to <strong>fail fast</strong>), had he/she kept the idea to him/herself only to find out about the issue when the system is already built.</p><figure class="kg-card kg-image-card kg-card-hascaption"><img src="https://vinta-cms.s3.amazonaws.com/media/filer_public/85/dd/85ddeb93-cc8a-4626-a8ea-be41711082a2/9_positive_feedback.png" class="kg-image" alt="alt text" title="Example of constructive feedback."><figcaption><em>Example of constructive feedback.</em></figcaption></figure><p>Such environment also inspires confidence and perpetuates positive feedback attitude. The same way theories say that a bullied kid is more likely to become a bully his/herself, if you’re rude with people in your pull requests, especially to people who are just starting, it’s more likely that they’ll grow accustomed to deal with PRs that same way, perpetuating the negativity.</p><p>I myself have been in real-life situations where badly provided feedback (even though it was meant to be constructive and positive) caused a great fuss and quite some anger thrown at each other. I was in a brazilian college mailing list (which means people were not native english speakers) where people were talking about “driver” when they should be using “drive”. Since this is a pretty common mistake I’ve seen, I decided to, as gently as I could, educate the guy about the difference between the two words. However, even though my intentions were good and I was walking on eggshells while typing, I’ve corrected him in an email sent to the whole mailing list, instead of just to him, which made it look a bit humiliating. He got pretty offended by it and in turn offended me, which started a pretty silly internet fight (as they always tend to be).</p><h3 id="different-roles">Different roles</h3><p><a href="https://www.youtube.com/watch?v=PJjmw9TRB7s">Some people</a> suggest that, in a pull request review, there are two roles: Requester and Reviewer.</p><p>As an author/requester, you’re supposed to briefly describe the issue being solved, don’t just point to the card/issue link. When you put the issue’s link, you’re making the reviewer have to hunt for the explanation through discussions in it, instead of giving him/her some context. You should always put the link, but don’t rely solely on that.</p><p>As a reviewer, you’re supposed to ask questions instead of making demands. Don’t go into the code and write “do this”, “fix that”, “remove this”. Ask why the person made it that way, there might be a reason you’re not seeing. If it’s something you’re sure that should be changed, try to pose it as a question instead of a command. Use “Is this being used somewhere? I think it can be removed” in place of “remove unused variable”. You’re not a linter to give imperative instructions, nor you’re talking to an AI assistant. You’re a human being talking to another one.</p><figure class="kg-card kg-image-card kg-card-hascaption"><img src="https://vinta-cms.s3.amazonaws.com/media/filer_public/02/b4/02b42838-2efe-48b6-94c6-028d7b0e922d/10_asking_questions.png" class="kg-image" alt="alt text" title="Asking questions allows the PR creator to explain his/her reasoning."><figcaption><em>Asking questions allows the PR creator to explain his/her reasoning.</em></figcaption></figure><p>Just make sure you can clearly see the difference between being polite and sounding judgemental. Asking “Why is this variable doing nothing?” will not make the mood better and could be replaced by “I don’t see this variable being used. Maybe it could be removed?”.</p><h3 id="minimum-of-2-approvals-per-pr">Minimum of 2 approvals per PR</h3><p>Obviously, the number in this section’s title is entirely subjective, depending on the team’s size. The idea here is to define a guideline for a minimum number of approvals per PR, so that we won’t have 2 people doing pair programming and reviewing each other’s PRs. I’ve worked in a few projects where parts of the team were geographically separated and sometimes the PRs would be reviewed only by the folks that were in the same region as the PR creator, even though the whole team was working in related features. This not only gives room for more bugs to pass unnoticed, but also creates a weird atmosphere within the team, a feeling of separation that breaks unity.</p><p>Furthermore, this practice ensures that at least Y + 1 people (where Y is the minimum number of PR approvals required) know the code being merged. Once again, reviewing code is sharing (and spreading) knowledge, both technical and about the feature. On this same note, there’s an <a href="https://www.youtube.com/watch?v=fatTnX8_ZRk">Atlassian talk</a> where they suggest that you always assign around 1.5x to 2.5x Y reviewers to the PR, so that it won’t get stuck waiting for a review from someone who’s already overloaded with requests. Once again, the availability of this idea depends on the team size.</p><h3 id="make-use-of-git-blame">Make use of git blame</h3><p>This is a command that I don’t see people using very often, unless a bug has happened and they’re looking for the culprit. I don’t blame them (sorry, couldn’t help it), the command’s name is quite aggressive. However, another good use for it is finding who to assign as a reviewer of a PR. Git blame will show you who’s already worked on those files, so they’ll probably be the most adequate reviewers for your PR.</p><figure class="kg-card kg-image-card kg-card-hascaption"><img src="https://vinta-cms.s3.amazonaws.com/media/filer_public/b5/b1/b5b1a611-3c72-4b93-bf38-d1b94ba193d3/11_git_blame.png" class="kg-image" alt="alt text" title="Filipe’s probably the best person to assign for this file’s review."><figcaption><em>Filipe’s probably the best person to assign for this file’s review.</em></figcaption></figure><h3 id="add-screenshots-for-ui-ux-changes">Add screenshots for UI/UX changes</h3><p>The title here is pretty self-explanatory. If you made some UI/UX changes, they might not be obvious for whoever’s reviewing it, especially if the person doesn’t work with that specific part of the software on a daily basis (and even so, oftentimes details pass unnoticed when we look at the same screens many hours a day). Make sure to add some screenshots, everybody loves images.</p><h3 id="5-good-practices">5 good practices</h3><p>Yo, dawg, I heard you like good practices lists, so I put a good practices list inside this good practices list. Here are 5 items extracted from that <a href="https://medium.com/@real_tadas/theres-a-human-on-the-other-side-of-your-code-review-9732cc15bfee">aforementioned article</a> which I consider good practices that can be easily done and summarized:</p><ul><li>If possible, before the review, talk to the requester (but keep it under 5 min)</li><li>Gives you an opportunity to understand the feature, the reasoning behind the code, the proud accomplishment of the developer (and you might learn a thing or two).</li><li>Let the automated tools do the nit-picky observations</li><li>Configure linters, CIs, automated tests, code coverage metrics so they point the tiny issues instead of wearing out your relationship with the developer over that.</li><li>Remember that nobody likes to be criticized (especially over small stuff), so let the tool take the blame.</li><li>Be nice on your phrasing</li><li>We’re used to be dry and straight, but that’s not always helpful, so at the very least, start your suggestions with a “please”.</li><li>Try “Could you explain this part to me?”, you might learn something.</li><li>This way, reviewees won’t be scared to innovate and will think of “what’s best” instead of “what my reviewer will say”.</li><li>Teach, don’t just tell</li><li>Remember, reviewing code is a moment for knowledge sharing, not just bug finding.</li><li>If you reject the PR, teach how it should be done and/or point to some documentation.</li><li>You shouldn’t be a code safety net but a mentor instead.</li><li>Finally, when something breaks, share the fault</li><li>You’ve reviewed that piece of code, if there’s a bug, you also let it pass. The developer is already under pressure for having his name in the commit, having to figure out a fix for the issue. Sharing the fault will make things easier for everyone.</li></ul><h2 id="coming-next-">Coming next…</h2><p>Phew! This was a pretty long part, but hopefully you've learned something new and were able to visualize how some of these tips could be applied to your workflow. In the <a href="https://www.vinta.com.br/blog/2018/pull-requests-merging-good-practices-your-project-part-3/">next (and last) part</a>, I'll be presenting some general tips, including tools, that can help you in the code review process, along with some code review examples from big projects. Stay tuned!</p>