Reviewing a Pull Request is always a challenge: how do we keep quality and code consistency while keeping developers motivated? Does labeling Pull Requests messages make things easier to interpret?
Recently, I tweeted about the https://conventionalcomments.org/ website. The idea is simple: every time you review a PR, your messages should start with a label, making it clear which is the next expected step (if any.)
What is a Pull Request (or Merge Request?)
A Pull Request is a request to add some code to the project’s “official” code. Depending on the tool, these requests can be called Merge Requests.
It goes like this:
- Person A:
- Writes some code for a specific change or addition
- Opens a PR describing what and why has been changed;
- A revision and adequation process happens. Part of this process can depend on a Person B review.
- The code is merged.
This post refers to messages from Person B to Person A explaining the changes needed before the code can be merged.
DISCLAIMER: There is a lot of material about PR etiquette. This post does not cover this specific topic but I do recommend you to search about it. Although just vaguely related, I recommend you to watch this video by Dave Farley about leadership and how important it is to allow people to solve problems in ways you would not do it.
Prefixes and labels: Agreements about the format
The website suggests the following format for revision comments on Pull Requests:
<label> [decorations]: <subject> [discussion]
For example:
suggestion (test,if-minor): It looks like we’re missing some unit test coverage that the cat disappears completely.
I am not sure if this is a big problem or not. Would it be possible to add tests for that scenario before we merge this?
In the example we have:
Label (Ex.: suggestion):
The suggestion label indicates this is about a possible improvement. What and why should be changed.
Decorators (Ex.: test, if-minor):
A quick way to expand the label meaning. The comment is about tests and the decision of whether it should block or not the merge is up to the PR author, depending on the size of the problem.
Subject:
The main message of the comment.
Discussion:
It is not required but can help to further explain the subject.
The website has very good suggestions for labels and decorators.
Two highlights: nitpick and praise
From the entire list, there are two that I found very interesting: nitpick and praise.
The nitpick label is a good way to say “This is not wrong but I would do this differently”. I recommend you to pay attention and not overuse this but this label can be a good way to share knowledge.
The praise label is for compliments. This is one way to recognize a good, creative, and/or elegant solution. As each comment creates a thread, the team can agree that the author can “resolve” it right after reading it.
What do you think about this? Share your thoughts in the comments section below or via social networks!
