How to raise a pull request (PR) as a beginner
Code Reviews are one of the major things that a developer has to do on a daily basis. For some, the experience of reviewing code can be delightful but for others it can be quite stressful!
According to Stack Overflow, the number one cause of stress in a developer’s life is code reviews.
- Just Kidding!
Code reviews or Pull Requests (PR) reviews is the process of collaborating on a piece of code to:
- Approve changes
- Request changes
- Comment on the changes
The process is important to ensure a healthy codebase. Without a formal PR review process, you’ll end up with a pandora box of surprises! Sometimes these surprises makes to production!
There are certain things that make the PR reviewing process difficult. Let’s go into detail about some prominent issues and how to avoid them.
No Clear Reviewer or Assignee or Title
Whenever you receive a PR to review and there is no assignee/reviewer added, it becomes difficult to track all the updates and changes to the PR. Therefore before opening a PR, always add details of the reviewer and assignee.
Check out the official docs to understand how to add reviewer and assignee to your PR.
Ensure the title of your PR is self-explanatory as this will appear in git history when the PR is merged. Do not add vague and gibberish things that make sense to you, but not others!
Delayed Reviews
Everybody is busy with their work. Understandable. But we need to understand our position when we are a reviewer for a PR. The assignee might be waiting for a review/go-ahead on a PR so that they can continue with their work.
In bigger teams, it is often seen that PRs are in review for far too long. Not hours, but days. Teams must come to a Service Level Agreement (SLA) for reviewing PRs. This is the time elapsed between a PR is raised and it is reviewed. For some it can be 1 hour, but for other it can be 5. This sets the right expectations for all stakeholders.
One of the major tools that I am a fan of is Slack Reminders. You can set alerts for all your PRs and it will send little reminders on a slack channel. You can read more about it here: Managing scheduled reminders for your team.
Missing Details — Pull Request Templates
In this remote-first world, the conversations and the work happen asynchronously. Thus, it is highly possible that the reviewer of your PR is sitting in a different time-zone.
Now, if your PR has inadequate information, then the PR review will get delayed by minimum 24 hours. Hence, your PR needs to have all the necessary information.
If you are confused and do not know what a PR template should entail, check this little take on pull requests:
Bad pull requests are unclear. They don’t have clear answers to “why are we doing this?” and “why are we doing it like this?”. They are usually unreasonable in size and include multiple changes to multiple subsystems. The reader is typically unsure if the pull request is mergeable, nor does he have any metric to help answer this question.
- Igor Šarčević
By “necessary information” I mean all the details the reviewer of the PR will need to test out the PR. This can be:
- Module for which the PR makes changes to
- Some background — Why and How
- JIRA Ticket attached to the PR
- Necessary testing steps
- Any specific environment changes the reviewer should do to review the PR
It is difficult and an unrealistic expectation to have from developers adding these details every-time they raise a PR. Hence, use another feature — PR Templates.
Pull Request Templates are helpful in reminding developers to fill the necessary details before submitting the PR. It can be as simple as having a reminder to “self-review” the PR and as complex as adding an .env file and testing steps.
This small template can save you hours of back-and-forth with the assignee of the PR and hence speed up the process. To see how it changes the whole experience of PR reviewing, I have filled a PR template below.
See how easy it becomes to review the PR now. At least, start with the review.
Repeated/Skipped/Ignored Reviews
This happens majorly with the developers who have just joined the workforce and hence tend to repeat the same mistakes they have already been pointed out. This makes PR reviewing a task and a bad one!
So, if you are a newbie and are getting review fixes around best practices or logic in one of your PRs, please ensure the subsequent PRs do not have the same mistakes. This point can be eliminated by a simple but thorough self-review.
The other side of this is when the reviewer gave you a substantial number of reviews and you skip them. Skipping them is still an honest mistake, but if you ignored it on purpose, that is just not acceptable. You need to understand and acknowledge the amount of time and hard-work the reviewer has invested. The reviewer wants to learn. Ignoring the ones you think are not substantial is just not cool!
The remedy to this is having an “acknowledgement system” for all the reviews. Whenever you fix the PR reviews and comments given, just leave a 👍 (thumbs-up emoji) or “Done” on that conversation. It won’t take more than a minute, but will go a long way in this two-way communication.
Singleton Purpose Of The Pull Request
One of the thumb rules that you have to follow is not to cover too many fixes and random things in a single pull request. Keep the work in the PR scoped to what the PR was intended for.
There are times when the perfectionist developer with ADHD in you cannot stand code smells. Just because it’s a minor fix, you try to squeeze those fixes in your PR too. Yes, you did a great job, but that was not the job you were given. The PR should be precise.
These little things (a.k.a habits) do improve the code base but at the cost of increased time of PR reviews. You can inform your reviewer about these code-smells. Do not increase the scope of the PR unnecessarily.
No PR Should Be More Than 400 Lines Of Code — Scientifically Backed
There are companies who instantly reject PRs if it has more than 500 lines of code. Igor Šarčević very beautifully puts this point across with his take on Pull Requests are a reflection of your engineering culture.
The longer we stare at something, the more blind to it we become. After an hour of looking at the same thing our brain becomes saturated and less effective at spotting issues.
Based on this data and the finding that review efficiency falls after 90 minutes, they concluded that a reviewer will be most effective reviewing no more than 400 lines of code in one go.
- Proof your thousand-line pull requests result in more bugs
Yes, there can be instances where you would not be able to follow this cardinal rule of 400 lines of code. But believe me, you can. Proof your thousand-line pull requests result in more bugs post underlines the ways we can spilt the PR with more than 400 lines of code and still not break the PRs.
In brief:
- Separate out refactoring
- Lay out the foundation of PR in 1 part
- Use feature flags
- Commit by portion of code and not based on time is another analogy by Pauline Vos in her talk Git Legit (Atomic commits will help you git legit). If you are interested in reading more on Atomic Commits, check this out.
Mind Your Tone Of Writing
Be polite. Be respectful. Be mindful. Everyone is learning here and everyone commits mistakes (pun intended). Hence, mind your tone of writing while you are giving out reviews or you are replying on the given review.
DO NOT USE ALL CAPS to show you are frustrated and are shouting (imaginary)
Give compliments wherever it’s due.
Thank your reviewer once a long PR is getting merged after 3–4 back and forth. Thank the reviewer if you have learnt anything new.
Thanks for reading ❤
If this blog was able to bring value, please follow me here! Your support keeps me driven!
Originally published on adityatyagi.com
Want to connect?
Follow me on Twitter and LinkedIn or reach out in the comments below!
Top comments (1)
I have to be honest, I came here for the "everyone commits mistakes" pun 😆
Fwiw I like it that in Gerrit you cannot mark a comment as resolved without replying. There are shortcuts for "Done" and "Ack" but somehow have to tell why you close the discussion.
(There are more aspects of Gerrit to be liked besides that: there are no stray comments/replies, always "reviews" (in the GitHub sense), your pending replies can be auto submitted when you push an update, reviews are commit-based rather than branch-based so it incentivizes "atomic" reviews: you can still put too much in a single commit, but generally people put them in separate commits in a single branch, etc.)