Why should PRs/MRs weigh more in software development
PRs? MRs? What?
For those of you who are new to software development, Pull Requests (PRs) or Merge Requests (MRs) are…well requests that you make in order to get your newly added/modified code into a parent branch. If you’re at the beginning you might not have done this if you worked alone on your personal projects, and just commited everything on the master branch, but in “real life” projects, where teams have more than 1 member, this is the AS IS way of doing things, to keep the already chaotic process of development under some kind of control.
But WHY..
Let’s get a bit more into how should PRs help developers.
For the last 3.5 years, I’ve been working on the same ecommerce project. It was huge, with hundreds of people working on it in the same time, that means that every day dozens of PRs were raised which introduced from a couple to thousands of changed / newly added lines of codes. The only, viable, way of making sure no (too serious) bugs were introduced, was by making code review and approved by at least 2 other members of the team before merging in the changes.
Now, imagine one of your colleagues worked on a feature that’s quite complex with lots of changes. Let’s assume both of you read the Clean Code (by Robert Martin) and you know that naming and modularizing your methods is quite a big deal when it comes to understanding and keeping your code clean. But even so, to do a good code review would require you to go from start to end of all the code flows that your coworker changed and make sure they make sense.
This is quite a hassle for everybody, I can assure you. It leads to a bunch of time wasted, frustration and sometimes you might understand the things in an incorrect way but you will assume that’s alright because that’s what the author wanted to do.
Or, you could work on a new feature that interacts with an older part of the project. While working on it, you might come up with some questions about why certain things were done in such a shady way. You’ll check the git history and see who made the changes and (for the sake of this article, let’s assume that person is no longer on the project) you’ll go for the PR/jira ticket to find out more details. If you’re unfortunate enough that the ticket doesn’t provide you with any technical details, you’ll head back to the PR and check whether or not the author left some notes that would make your life easier while reviewing his/her PR. If this last one is unsatisfactory or worse, EMPTY 😱 … RIP 😄
The perfect PR
In my time as a developer, I’ve come across multiple forms of PR design, some were too verbose, some were too shallow, some were not existing at all 😅 (that’s not necessarily a bad thing sometimes 🤐) , so from my point of view, there is no such thing as a perfect PR design (like that surprises you), but here is what I think should be the minimum of information a PR should have.
What does this PR do?
- Here, ideally, you should have a list of changes, additions, removals etc. that you do in your PR
- It should be concise, not necessarily short. Sometimes you need to be a little more verbose in order for your audience to understand your logic
Coverage Evidence
- This is an optional section. If your project practices unit testing, you should add a proof of your tests passing and the code coverage results
- This is mainly to make it easier for the reviewer to know that your changes are covered and tested
Changes Screenshots
- For backend only changes ( here I’m talking about changes that don’t necessarily reflect in some UI component) this section is also optional
- But in case you’ve made changes that affect UI components, this step becomes mandatory
- It’s really hard to tell how the code will make the UI look if the changes are CSS/HTML or part of some other frontend framework. The reviewers would be very pleased to see these here
How can this be tested
- I’d say this is a semi-required step
- It comes in handy when the changes are a little bit more complex, but nonetheless, even if the changes are small, this step will greatly help new comers on the project
Technical Design (TD) link
- I haven’t written this down in the screenshot but in some bigger projects, architects/technical leads/seniors are doing Technical Designs of features, then the devs implement them. It would be a good idea to link them on the PR to enforce your reasoning of implementing your changes the way you did.
Reviewers
- Please always add at least 2 reviewers to your PR and only merge it when you get at least 2 approvals. I’m not sure if any development guru considers this as a best practice, but I sure do.
- More eyes on a some piece of code, the better
Conclusion
So my purpose when writing this article was to make you be more aware of the importance of (well written) PRs/MRs. On the long run they become an essential component of the project, especially when people are joining/leaving the project, but not only that.
- They make it easier for others to understand your implementation logic
- It saves time when one is reviewing a PR
- Gives the project’s development flow a more steady feeling
- Developers will know where to find useful information about a certain change and the logic behind it
Please don’t treat them lightly. ✌️