Pull Requests: Merging good practices into your project (part 3)

<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 or the second one, <a href="https://www.vinta.com.br/blog/2018/pull-requests-merging-good-practices-your-project-part-2/">click in the links to read it.</a></p><p>After such a long list of good practices in the last part, I'll present now some more straight-forward quick tips for improving the code reviewing process. You'll also see some real-world examples of code review from the guys at Django Project. And finally, you'll be presented to an extra treat to help you in your own projects. Let's get started!</p><h2 id="tips">Tips</h2><ul><li>On GitHub, you can put some keywords in the PR (or in the commit messages), along with the number of the issue that the PR refers to, in order to <a href="https://help.github.com/articles/closing-issues-using-keywords/">close it automatically once the PR is merged</a>. For instance, if you put “Closes #3244” in your PR, issue #3244 will be automatically closed once your PR is approved and merged, which helps keeping the repo clean and organized.</li><li>GitHub also allows you to create permalinks (direct links) to specific parts of the code, so in case you need to reference a given part, you can just send the link of where it starts and where it ends, instead of having to textually describe it.</li></ul><p>There are also some pretty interesting tools that help a lot in the review process, be it locally in the PR creator’s computer, on the web during the review or even in the backend side. Here are a few of them:</p><p><a href="https://github.com/pd4d10/octohint">Octohint</a>, <a href="https://github.com/refined-bitbucket/refined-bitbucket">Refined Bitbucket</a> and <a href="https://github.com/sindresorhus/refined-github">Refined GitHub</a>:</p><ul><li>These are browser extensions meant to add nifty little features for when you’re looking at code directly in the repository, like syntax and occurrence highlighting, keyboard shortcuts, issue/PR templates, etc.</li></ul><p><a href="https://www.atlassian.com/software/crucible">Crucible</a>:</p><ul><li>This is a code review tool made by Atlassian that helps a lot with this process, bringing some nice features like displaying what changes were made after the last comments you’ve made in a PR (so you don’t have to review everything again), keeping track of which files you’ve reviewed already in case you stop the review halfway, etc. It’s a paid tool, however, and I’m not sure if the feature support is the same across all repository providers (GitHub, Bitbucket, GitLab).</li></ul><p><a href="https://devcenter.heroku.com/articles/github-integration-review-apps">Review Apps</a> (Heroku) and <a href="https://www.netlify.com/blog/2016/07/20/introducing-deploy-previews-in-netlify/">Deploy Previews</a> (Netlify):</p><ul><li>These are solutions on both hosting providers that automatically create an instance of the system with the changes made in the PR. You get a unique URL through which you can navigate through the system as if you were actually using the production link, but with the newly implemented changes, so you can test them in a real-life-like environment. These are also paid features.</li></ul><p><a href="https://www.youtube.com/watch?v=IMRHFlDxaqU">Linters</a>:</p><ul><li>Linters are awesome tools for keeping the code organized and well structured, which in turn makes it way easier to understand and review. These can (and should) be configured both on the developer’s local machine (and added to pre-commit configurations, preferably) and on the deployment servers. The link in this subsection’s title points to an awesome talk about a plethora of linter types, by Flavio Juvenal, from Vinta Software.</li></ul><h2 id="real-world-examples">Real world examples</h2><p>Of course, even though I’ve provided lots of links and references wherever possible, tons of theory won’t really be absorbed without some practical examples of do’s and don’ts. Here are some examples of how big projects are reviewed and which kinds of tools they use to improve this process. I’ve also added some examples of good practices we do and issues that we’ve had at Vinta Software, the company that I currently work at.</p><h3 id="examples-from-django-project">Examples from Django Project</h3><figure class="kg-card kg-image-card kg-card-hascaption"><img src="https://vinta-cms.s3.amazonaws.com/media/filer_public/77/d9/77d91d7f-f108-4eb5-ade9-32a7031df6a5/1_django_project.png" class="kg-image" alt="alt text" title="Django Project's example #1"><figcaption>Notice the use of “please” and the explanation about simple things - like how the conflicts are represented in code - without being condescending.</figcaption></figure><figure class="kg-card kg-image-card kg-card-hascaption"><img src="https://vinta-cms.s3.amazonaws.com/media/filer_public/66/b5/66b517b8-8845-4013-8b99-010451444f50/2_django_project.png" class="kg-image" alt="alt text" title="Django Project's example #2"><figcaption>Again, putting suggestions as questions, not as imperative sentences. Also teaching about Python 2 vs Python 3, sharing knowledge.</figcaption></figure><figure class="kg-card kg-image-card kg-card-hascaption"><img src="https://vinta-cms.s3.amazonaws.com/media/filer_public/44/2e/442eeb2e-160f-4553-b9f7-2b484a957e6d/3_django_project.png" class="kg-image" alt="alt text" title="Django Project's example #3"><figcaption>Politely requesting the user to check the guidelines before submitting, so the process can happen smoothly.</figcaption></figure><figure class="kg-card kg-image-card kg-card-hascaption"><img src="https://vinta-cms.s3.amazonaws.com/media/filer_public/9d/ae/9daedc1d-210e-4911-987b-c6c71ccee31d/4_django_project.png" class="kg-image" alt="alt text" title="Django Project's example #4"><figcaption>Starting with “please” and indicating clearly where changes should exist. Also asking questions about the requester’s intent instead of pointing it as errors, which gives him a chance to explain the rationale behind his thinking.</figcaption></figure><h2 id="vinta-s-insights">Vinta’s insights</h2><p>If you’re submitting PRs to people inside the company/organization you work at, always warn the reviewers on Slack when the PR is created, reviewed and updated. People usually get tons of emails daily, so don’t rely on them being notified automatically by the repository service that you use. It also makes things a bit more personal, which is always nice.</p><p>When you’re assigned to review a PR, make sure that you always test the feature first, and only then review the code (remember, <strong>fail fast</strong>). Testing the feature is almost always (or should be, at least) easier and faster than reviewing the code. If the feature is not working properly, there is no point in reviewing the code, since it’ll certainly change.</p><h3 id="vinta-s-lessons-learned">Vinta’s lessons learned</h3><p>Writing this article on good practices does not make me or Vinta immune to making mistakes or facing challenges due to code management issues. Here are a few of the one’s we’ve already had to deal with.</p><p>On a project I was working, we decided to have the “close branch on merge” checkbox checked by default, meaning that whenever a remote branch was approved to be merged to staging, it would be closed/deleted after the merge. However, one day we had to deploy urgent features from staging into production, but staging had some other features that were being tested by the client and we didn’t want them to be deployed. The merge commit of that branch into staging was behind the commits for the features we wanted to deploy, and the feature under test didn’t have a branch anymore, so I was concerned that it could get lost in the commit revert. Since I had never done something like that before, I was quite worried that the feature would get lost. In the end, it was no big deal and everything went fine, but from this day on, I don’t have the “close branch on merge” checked anymore and I only delete the remote branch once the feature has been merged into production.</p><p>On another project, we’ve faced a scarier problem, due to lack of knowledge of how one of GitHub’s features work and some lack of consistency on GitHub’s part regarding its settings. A person was merging the master branch into a feature branch in order to get the most up-to-date version of the code, but he was doing it through GitHub’s interface, not through the command line. GitHub pointed out that there were some conflicts in the merge. The person looked at them and saw that GitHub’s suggestion for fixing the conflicts was correct, so he clicked on the “Resolve conflicts” button. However, GitHub’s behavior for that button is to fix the issues and commit the merge into the source branch, which, in this case, was master. What it meant was that the feature branch was actually merged into master, which was connected to a CI service and ended up deploying the feature, unbeknownst to the person. It was only found out some time later, when the system started having problems because of this unfinished feature that was deployed. While it was partially our fault for not knowing exactly how GitHub’s “Resolve conflicts” button works, the project in question did have lots of safety warnings and confirmation screens whenever you wanted to merge something into master directly. However, in this case, when clicking in the “Resolve conflicts” button, it simply merged without throwing warnings about it, causing the whole confusion. Ever since this has happened, the folks responsible for the project have disable the option of resolving conflicts through GitHub’s UI, to avoid future issues like this. So for those of you who are reading, I highly recommend disabling it in your repositories as well, unless you’re fond of thrilling unexpected experiences.</p><h2 id="conclusion">Conclusion</h2><p>We've finally reached the end of this 3-part article on best practices for pull requests. I hope this content has been useful and that you feel like you've learned something new. A comprehensive review never actually covers a given topic in its entirety, so please let me know if there's something you think is missing from my post. Also, feel free to share your experiences (good or bad) regarding pull requests, it's always soothing to find people who've passed through the same problems. Thanks for sticking with me for this long and I hope to see you again in the next article!</p><h2 id="one-more-thing">One more thing</h2><p>So, in the most Apple Keynoteish-style, here’s one more thing for you. We at Vinta are huge fans of checklists, so we don’t have to rely on our flawed memories. During the research and writing of this article, <a href="http://pullrequests.devchecklists.com/">we’ve developed a checklist</a> that tries to provide an easy and quick set of things that we recommend you to keep in mind (or configure accordingly in your repos) when dealing with code management and pull request reviews. As I said in the very beginning about this article, this checklist is also meant to be only a guideline, not an absolute source of truth. Not everything in it might apply to your project, so use it as suitable. And yes, the checklist is completely free to use in any way. I hope you’ve enjoyed reading this and that it was of some use. Feel free to get in touch if you want to discuss more or suggest/ask something. Cheers!</p>

Pull Requests: Merging good practices into your project (part 3)

Join the Tech Forward newsletter

Related articles

Testing the diff

DjangoCon US 2019: Python & Django in San Diego!

Vinta at PythonBrasil 2021