- Coding is work. Reviewing is, also, work. The added value of the code resides in its usage. Code is used when merged, merged when reviewed. So keep in mind that reviewing is adding value.
- Every member of the team should have at least one dedicated time slot (two or more is healthier) during his regular working day to perform code reviews. For example, at the beginning of the day, before starting something else, take a tour of GitHub. In the same way, when coming back from lunch.
- To know what is waiting for a review, the Review Request can be helpful (but sometimes too noisy). Take time to configure your Notifications, it's worth it.
- Even if you are busy working on the next big thing, don't let others wait for you, ensure that everyone can work at their full potential.
- The team owns several projects and the whole team is notified when a pull-request is opened. Once a review has been started by someone, it disappears from the "Review Request" tab. So, once you start a review, you are responsible to review it once merged or closed.
- There is a GitHub feature for load-balancing review between the member of the team, but it requires some discipline on each member, declaring your time off. It should be used as a last resort if we fail to organise by ourselves.
- GitHub has a built-in notification system which can be powerful if used properly.
- If you're not a fan of GitHub UI and prefer having all in Slack, you can configure Scheduled Reminders to receive it where you want.
- If you're not a fan of GitHub UI and prefer receiving emails, you can also configure Email Routing alongside with Notifications
-
A pull-request is about proposing to change code (adding, removing, updating), in an asynchronous way: be verbose. Explain why, what and how you change things, if the code is not self-describing.
-
Before asking someone to review your code, review yourself. Debug prints, temporary values, and TODOs happen. Clean them up before requesting review.
-
Your commit history should tell a story. Even though they will be squashed after merging, it helps your reviewer to understand your path to the given changes.
-
You can add more semantic to your commit by following some conventions like Conventional Commit, GitMoji. Whatever you prefer, it's always better than "adding feature", "working stuff", "quick fix", "feedback fixes".
-
When you push new changes to a pull-request which review has started, it notifies The Reviewer. Turn your pull-request into a draft if there is still ongoing work.
-
Ensure to push code that complies with formatters, linters, tests, etc. Test your code locally. Value others' time. Don't let the Continuous Integration tell you that formatting is wrong, you can fix it before. Don't let The Reviewer find linter or compilation mistakes.
-
Don't mix things. It's already hard for The Reviewer to jump on an unknown topic, don't add multiples topics. Separate in commits, separate in pull-requests, based on the size of changes.
-
Size matters. It's generally quicker and easier to review a small batch of changes (~500 lines of code) than a big change all at once.
-
Don't stack your work, if you have more than 5 open pull-requests (this number is arbitrary, everyone has their own), kindly remind to The Reviewer that you are stuck. Stop starting, start finishing.
-
You are not the code you write. If someone reviews your code, it's the code that is reviewed, not you. Easy to say, not easy to do, but don't make it personal, it shouldn't. In the end, the code belongs to the team.
-
The goal of the review is to ensure quality and consistency of the codebase for the repository, the team, etc.
-
Don't resolve conversation yourself. The Reviewer asks for something, that's his responsibility to mark the conversation as resolved, based on your modification / answer. It's easier to follow. Notable exceptions to this are:
- If the comment was a suggestion you accepted, it will be resolved automatically, and that's fine.
- If the comment was without any ambiguous interpretations (e.g. "small typo in the variable name") and you made the change, it's ok to resolve by yourself.
-
Before merging your pull-request, ensure that all comments have been addressed / resolved, that the continuous integration is green, review the description for any warning comments (e.g. "merge the Infra-as-code pull-request before this one").
-
Once a review has started, avoid pushing force on the branch. GitHub has a feature "New changes since you last viewed" that help The Reviewer to read only new changes instead of the whole pull-request.
- Unless your changes are critical (ongoing incident, security issue, etc) you shouldn't have to beg for a review on a corporate chat. Someone will pick it sooner or later. Everyone is working, in an asynchronous way and does his best, patience is key. After one full day, if no one have picked your pull-request, a kind reminder to the process can be necessary 🤗
- We should trust each other as equal in the review process. Unless a specific member of the team is mentioned in the Pull-Request description, anyone should feel comfortable to start a pull-request review.
- When you don't know, you don't know, don't be afraid to ask for help by mentioning people.
- Do not assume people know or have bad intents, ask genuinely and keep a learning/teaching mindset.
- If multiple people go on the pull-request at the same time, the Review workflow is not efficient. Assign yourself before starting the review. It lets other know that someone is on it.
-
Review the code, according to the context given by The Developer. Don't review The Developer. Ask yourself "in 6 months or more, will it be understandable?". Code is read 10 times more than written
-
Leave your ego at the door. If The Developer changes the code you wrote, don't make it personal. Again, we review code, not people. Humans make mistakes, you are a human.
-
Try to categorize your comments with ConventionalComment or any other expressive approach. It expresses your intent when leaving a comment, and help The Developer to accept the feedback under a certain prism.
-
Submit all your comments at once and choose your review score accordingly.
- Approve is allowing people to merge, regardless of the comments you made. The Developer has to consider them, in order to keep a good trust within the team, but if the pull-request has auto-merging, it will be merged as soon as it gets the approval.
- Request changes is blocking the merge until you explicitly change your score. It should be used wisely: if there are notable breaking changes, a lack of trust or something wide to discuss before moving forward. Don't request changes before taking some vacations, it will block The Developer.
- Comment is not giving approval but not blocking, this is the default when none of the above apply.
-
If there are too many comments, or if you feel that discussing the matter live would be more efficient than exchanging asynchronous remarks, please request a pair‑review with screen sharing or a side‑by‑side review.