This content originally appeared on DEV Community and was authored by Lucas Wolff
A great pull request is not only about correct code. Even if your code works and it's aligned with its purpose, it can be rejected by the reviewer. There are many reasons why this may happen, and today we'll discuss how to reduce this friction.
The techniques below are not meant to be applied on the code itself; they consider that all the requirements are fulfilled i.e. you did your job correctly, and also that merge conflicts are fixed, if they existed.
1. Keep your changes small and focused
This is one of the most common mistakes when opening a PR. It's very hard for the reviewer to understand and get context of a lot of files and lines being changed.
A big PR may be the result of your team's agreed workflow, or even of the way that you get the stories. The result is you ending up with a big PR that contains a lot of stuff. This is not wrong, it's just harder to understand.
One possible solution to this is to have one main feature branch, and other related branches that represent the steps to create the feature. When you finish your work, you'll merge all children branches into the main feature, and the main feature into the repository's main. Let me show you:
-
main-
feature/cart-component(main feature branch)-
feature/cart-component/create-presentation(feature child) -
feature/cart-component/create-tests(feature child) -
feature/cart-component/fix-related-tests(feature child) -
feature/cart-component/update documentation(feature child)
-
-
When using this approach, a good practice is to add links to the children's PRs on the parent, and vice-versa. This will help to not break the expectations of the reviewer when he/she sees an incomplete PR. Remember also to always rebase the new child PRs with the updated feature branch.
It's also very common to add non-related changes to the PR, like small refactors, for example:
- Variable names;
- Function names;
- Indentation;
- Improve unrelated logic;
- etc.
Refactors are always welcome, but maybe the timing is wrong. If you choose to do these unrelated changes in your PR, be sure to add a comment on the updated lines explaining what and why you did it, and explicitly say that the changes are not related to the PR, but you took the chance to change. You got the idea.
2. All red / all green
I learned this concept with a past manager, and it helped me a lot on my PRs, and also on my code reviews.
The idea here is basically to have a PR where everything (or most of it is) is red or green. I know it's hard to do that, and sometimes can be impossible due to the nature of the feature or fix that you're working.
This is important because it will show upfront and at a high level what is being changed.
On the visual aspect, it's much more comfortable, easy to read and understand a change when everything has the same color. The intention is clear: you're adding or removing code, not replacing. This resonates with the idea behind the Open-closed principle (OCP):
Software entities should be open for extension, but closed for modification; that is, such an entity can allow its behavior to be extended without modifying its source code.
It's interesting to see the effect that this type of detail in your PR can represent at a software engineering level. It doesn't mean that every all green/red PR will be an application of OCP, but sometimes it can be the case.
If you can't have all green/red PRs, a good approach is to have commits applying this idea. This allows the reviewer to navigate between commits and understand their purposes much easier than if you have one huge commit per PR. I'll talk about that in the next topic.
3. Using commits as a documentation
When hiking, you need to strategically place anchors in order to prevent you from falling and losing your progress. When creating commits this is no different—apart from risking your life.
Ideal PRs have commits placed correctly. If you mess up your commits, you can cherry-pick them and re-create your commit history. This can be done via CLI or even using some Git GUI, which is faster and easier.
The same logic used to create focused and small PRs can be applied to commits. The ideal commit serves one purpose, and it's a surgical change on the codebase.
Try to avoid writing generic commit messages like:
fix authadd cart featurerefactor componentupdate endpoint
These messages are almost meaningless when read without opening the change. They bring up some questions that can lead the reviewer to misunderstand the purpose of the change, and delay the review.
Instead of that, try to use commit message conventions and descriptive messages. Refactoring the examples above, we can have messages like:
-
fix auth→fix: correct authentication when token expires, -
add mini cart→feat: create mini cart componentANDfeat: toggle mini cart on header cart icon click -
refactor component→refactor: improve readability of UserList component -
update endpoint→feat: add pagination params to GET /users endpoint
These improvements took a small effort and represent a great improvement in your PR's overall quality, increasing the chances to be approved.
Don't worry if your PR has a lot of commits with long messages. At the end you'll probably going to squash tem into one commit containing the feature description and merge it to the desired branch.
4. Over communicate in your PR description
Imagine that you enter a bookstore and see a book cover that catches your attention. You're excited and ready to read the book summary, but wait... there's no summary! You need to read some pages to get the idea of the book, and after a couple of minutes you lose your interest.
That's what a PR without a description is. This logic can also be extended to the PR's title.
At the same time, you don't want to copy the story description; the reviewer can go to the story and read all the specifications. The idea here is to describe the PR at the code level. A good idea is to create templates that will be the default description for every new PR. Here is an example:
Related to [#TICKET_NUMBER](#TICKET_URL).
# What?
<!-- Explain the overall effect that this PR will cause. -->
# Why?
<!-- Related to business and/or engineering objectives and goals. -->
# How?
<!-- Explain the design decisions and how you solved them. -->
# How to reproduce
<!-- Guide the reviewer on how to test it. Be very specific here. -->
<!-- Be aware that sometimes extra actions need to be taken to fully test the PR. e.g.. add some env var. -->
# Screenshots or video (optional)
<!-- Good to show the final result of your work in a "human way". -->
# Extra notes
<!-- Extra comments that you may have. -->
<!-- e.g., "Need to update env vars on the server after the deployment." -->
# Checklist
[] Current branch is rebased onto main branch
[] Dependent changes (packages) are merged and published
[] Added the necessary tests
[] New and existing unit tests are passing locally
[] The code follows the style-guide
[] There are no linting warnings and errors
[] No console warnings and errors were introduced
[] Added comments on not so clear logic
[] Documentation is updated
The above template is only an example and it has no means to be the best one. My intention is to give you some ideas to create your own. Keep in mind that having a template is always a work in progress: you will need to improve it, remove some stuff, and add others. It should also be aligned with the repository's purpose (front or backend, for example), and also with your team needs and workflow.
5. Make the reviewer's job easier
Doing code reviews can be an exhausting task. If your PR is the third of the day, probably the reviewer will not have the same attention as if it was the first one.
That's why it's important to give the right resources to the reviewer. Doesn't matter the type of change that is being proposed, you probably can provide some extra resources to help the reviewers on their jobs. Here are a few examples:
- Screenshots;
- Videos (use async video services like Loom);
- Postman collections;
- Scripts to run external tasks e.g., database migrations, infrastructure changes;
- Specific testing credentials;
- Access to third-party services for validation purposes;
- etc.
Some of these resources can be critical to testing the PR (e.g., specific testing credentials). By omitting them from the reviewer, you increase the chances of having back and forth communication and a consequently delay on the review, or even a request for changes on the PR.
6. Review your own PR
You don't want to deliver to the world a code that was not even checked by you. Not only the code, but the PR's title, description and other subtle information that may not be contained in these places.
There is some information that may fit better if you put them as near as possible to the code. You can comment on the exact lines that you need, explaining behaviors more specifically and at a code level. A good example of the use of comments, is to explain changes not related to the PR e.g., small refactors.
This is the last step before assigning your PR to other people. Doing this you'll be behaving like a reviewer, seeing exactly what they will see during review. Here are some advantages of self-reviewing your PR:
- Catch bugs on the code;
- Ensure that all the requirements were fulfilled;
- Identify missing information on the description;
- Add extra information at code level (e.g., commenting on the lines);
- Exercise your code review skills;
You can also create a draft pull request while you have work in progress, and then convert to a PR only when you're ready to assign it to others. Draft PRs doesn't send notifications to the reviewers, and also there are no chances to accidentally merge the code.
7. Review other PRs to gain experience
Doing code review doesn't directly increase the chances of your PRs to get approved. By doing this you'll train your code review skills "in the real world", and as a side effect you will write much better PRs.
Many people recommend reviewing PRs as the first task of your work day. This is a good rule, but depending on your environment this could not work very well. Sometimes a bug fix needs to be merged at the end of the day, or a PR needs to be merged because it's blocking another. Try to have a routine, but be flexible to change it as you go. As a developer, in general, you need to be flexible.
Code review is a huge topic that has content for multiple articles, but here are some actionable tips that you can start applying now:
- Don't NIT too much, and if you do, don't block the merge. Every NIT is a chance to improve your linter;
- Be kind and don't use aggressive or demanding words;
- Be focused and don't multitask during the review;
- Understand all the context around the PR before starting the review;
- Communicate precisely to avoid back and forth;
- Timebox to be more efficient and don't spend too much time.
There are a lot of other techniques, but as I mentioned, this is a topic for another article.
I want to also highlight the importance of doing reviews for the teams' synergy and also to enhance your PR-building skill. This is something that will follow you over your entire career, so it's important to always keep leveling-up.
Wrapping up
Don't take the comments on your code too personal, and don't be passive aggressive on your answers. No one knows everything, and this is fine. By writing good pull requests, you will help to reduce the code review time, and collaborate to increase your team's performance.
Which other techniques do you use? Let me know, I'd be interested to learn!
If you have any feedback or suggestions, send me an email
Great coding!
This content originally appeared on DEV Community and was authored by Lucas Wolff
Lucas Wolff | Sciencx (2023-06-05T12:13:51+00:00) 7 actionable tips to get your PR approved (+ template). Retrieved from https://www.scien.cx/2023/06/05/7-actionable-tips-to-get-your-pr-approved-template/
Please log in to upload a file.
There are no updates yet.
Click the Upload button above to add an update.