Many developers are familiar with the situation like “where did this code fragment come from and why is it needed?”. You have to spend time and deal with the details already considered by another colleague. How to make it take a less amount of time? To achieve this, pay attention to a process of writing descriptions for Pull Requests (known as “PRs”) and Merge Requests (known as “MRs”). This article will focus on the content of the PR description without any explanation of coding since each project has its own coding-related specifications and requirements.
Here is the list of topics that are covered in the article:
First of all, let’s try to understand why it is better to pay attention to the description for PR.
Working on a project is a long path that’s divided into short segments. The beginning of each segment is a task, the steps are commits, and the end is PR.
The importance of PR is rising when you start to realize that PR is the result of your work on the task. Superficially, it seems that the most important part is the commits with code that introduces functionality, optimizes the work of the final product.
However, there is also another thing — the context of the changes that you apply to the product and the reasoning.
The context implies your rationale for the chosen implementation method, the selected tools, the encountered issues, and the dilemmas.
Providing context for PR is useful since the task itself and its discussion might be not sufficient enough to understand the full picture of the changes. It is good, when the discussion was conducted in the issue tracker or, verbally or in the chat, and was written in the form of short results.
If not, then you will have to answer the questions like
“where does this code come from and why is it needed here?”
As a reviewer, I don’t feel joy during a PR review if I have to spend extra time to understand the logic of code and commits, because the author did not bother to write a proper description. When the coding part is done, the author of the changes could dedicate some time to the general description of PR, while everything is still fresh in memory. In this way, the author would save the time and effort of the project team. Let’s list the project members, who can benefit from the properly described PR:
This all leads to a conclusion:
The description of PR is as valuable as the task ticket.
Presenting the results of the work in the description for PR is important and useful, as it will save precious time and energy for the future.
Now let’s consider important parts to cover to write a useful description for PR.
Start with the title — it should reflect the essence of the changes. If there is a corresponding task or ticket, then the PR title can overlap with it. In such a case, a prefix with the task number should be added.
SLI-001, SLI-002 - Support for reading input from STDIN
SLI-002 - Bugfix for memory leak when input is supplied via STDIN
The title should preferably be informative and short enough.
Tasks and tickets that are related to or will be solved by the PR must be provided. Better to provide them as a list with links to tickets in the tracker. In MarkDown, it looks the following way:
## Related tasks
- [SLI-001](http://my.issue.tracker/project/SLI-001)
- [SLI-002](http://my.issue.tracker/project/SLI-002)
Such an approach greatly simplifies navigation. For tasks that are located on GitHub and GitLab, there are special phrases that can be used in PR to close the referenced issues (manual for GH and manual for GL).
If the PR is based on other open PRs that should be reviewed first, then they should be listed. This also applies to PRs for used packages and dependencies. Here is the example in MarkDown:
## Depends on
- #3 (other opened PR in same repository)
- dependency#55 (other PR in the other related repository)
This will optimize the PR review process and save time for reviewers. If your PR does not depend on anything, skip the section.
This section is dedicated to preliminary research or other things that were preceding the applied changes. The following should be covered here:
If you made a choice between possible packages and chose the most suitable one, then you should provide links to each package, then describe its “pros and cons” for solving the particular problem. You should also state the final reason why you have chosen a specific package for the integration into the project.
If there was research about a task-related topic or an investigation for a different implementation approach, then you should provide links to the articles that were helpful in understanding the solution. You can also describe key points that will help to understand the changes that will follow.
If you discovered that the existing code is disturbing you, then it should be outlined. If you were forced to refactor some function due to it being poorly implemented or affecting your implementation in some way, then it would be better to explain why such change was considered as necessary.
Brief results of discussions with the team, which will help to understand why certain decisions were made.
An example may look like following:
## Premise
As we have grammar description and access to an
assembly code even from AST, I considered to use
PEG-based parser to generate custom AST from
assembly code, then use adapter to make it
compatible with current ASTReader processing logic.
As prerequisites I've looked through the following
packages:
- https://github.com/navstev0/nodebnf (npm) - I
tried and in the end came to the conclusion, that
it is not so stable to be used for our needs. Also,
lacks proper documentation.
- https://github.com/lys-lang/node-ebnf (npm) - Has
proper documentation and TypeScript definitions.
Also, has a useful online sandbox. Chosen for the
further implementation.
This section is used for general cases when it is necessary to present any information in a reasonable volume that the reviewer or any other colleague should know before they begin the inspection of the changes of PR. If there is no such need, then the section should be skipped.
The section should list the changes that are applied. If you previously described the details in the messages of commits, then you can reuse the text from there.
If you think that some points require additional explanation, then you can indicate important information as sub-items.
## Changes
- Added dependency [ebnf](https://www.npmjs.com/package/ebnf).
- Cleaned **package.json** from some obsolete
commands. Introduced `ebnf:update` to copy BNF-
files when post-install occurs.
- Introduced **src/ebnf** directory with components
to parse code strings by BNF definitions.
- Tweaked configuration to be able to work with a
new set of components.
- Introduced tests to cover new functionality.
Here are some guidelines for formulating the contents for the section:
Some changes are available via reviewing the diff. Therefore, do not micro-describe — GIT will do it for you. It is better to summarize the changes, stating their meaning. For instance:
- The dependency versions are updated because they
are very outdated and the security audit detected
vulnerabilities.
- Files in the X directory were deleted because the
component that worked with them was replaced with
another. They were no longer needed.
- The method was updated because the low-level API
has changed, and because of this, the method no
longer worked properly.
When justifying actions related to dependencies, do not hesitate to use links to releases, merged bug fixes or features — it is going to be very useful in the future.
If you introduce a new class, function, method, or other routines, it is better to describe its purpose (or even better, do it in the doc block in front of its definition).
Try to pay a bit more attention to the actions that cannot be covered by comments in files with the source code: moving files, deletions, automatic updates (dependency configuration, for example).
Some additional features, that you also should be aware about:
If you have any doubts or feel contradictory, regarding the implementation, then mention in PR. Feel free to state your concerns and ask for advice from project colleagues. Here is a short list of possible reason to mention:
Here is an example in Markdown:
## Concerns
It appears that the compiler is trimming spaces and
reducing indentation within assembly code strings
in `Assembly` AST node. This causes inability to
generate proper `src` attributes for custom AST by
simple math. Also, this is not fixable, because
these dependencies are already released and can not
be patched. Need to find another way to link
`AssemblyIdentifier` to a top-level
`VariableDeclaration` node. Currently, any
`AssemblyItem` node will have the same `src` as
parent `Assembly` node. Maybe I'm missing
something... I would appreciate any suggestions
that will help in solving the situation.
If you have not encountered problems, then you should omit the section.
This section lists all the aspects that are not obvious or are side effects. For example, you introduced a new build command that copies files, or used some kind of functionality with “magic numbers”.
Better mention such things in advance, because the questions will be asked sooner or later anyway. Here are some examples:
## Notes
- This PR changes major package API, which would
result in a backward compatibility (BC) break.
- When changes applied to grammar files, developer
must run `npm run ebnf:update` to copy updated
grammar files to **dist/**. Otherwise, components
will not react to any changes. All grammar files
are static assets, so they are not compiled or
handled by **tsc** in any way.
- For some reason `process.stdin.fd` is not
available in type definitions for the current node
version. It seems that we should upgrade at some
point. I resolved this via magic
`fse.readFileSync(0, { encoding: "utf8" })` (as `0`
should point to `STDIN` for each process) and left
a comment with the reasoning.
A few actions that will allow you to compose the useful PR description.
Please note that GitHub and GitLab provide the ability to create a PR description template (instructions are provided here for GH and here for GL). Discuss with the team if it is worth setting templates in advance.
When writing code, think about how you will compose the commit messages. Well-written commit messages are one of the best sources for the “Changes” section. Do not forget to mention the ticket code from the tracker. Modern trackers (Atlassian Jira, for example) are able to collect data from connected project repositories and generate links to related commits and PR.
Before creating a PR, put yourself in place of a reviewer. Create a PR with only two sections “Tasks” and “Changes” with a “Reserved” mark. Then do a code review by yourself.
This will help to look at the changes from a new angle. Please note that a less experienced colleague may walkthrough with your changes after the merge.
Ask yourself the question: “To not waste time in the future, is it worth giving an additional explanation in the description?”.
Think about what things might raise questions in the long term. If more than two people are involved in the project, then colleagues can start asking questions. If you can answer some of them in advance — this is a good reason to expand the description of PR.
Next, try to put yourself out of context a bit and think about what other aspects that may be important to the reader. The project may need to be transferred to other developers. In this case, it will be necessary to write instructions.
Ask yourself, “Isn’t it better to state important things now, while everything is fresh in my memory?”
Remember if some important implementation decisions were discussed with your colleagues verbally or on other resources. If discussions have taken place, then take a little time to describe the summary. Feel free to provide links to useful articles that you have read and which will help to understand the logic of changes or decisions made.
Consider to make the description more explicit. Pay attention to the possibility of using labels to highlight that your PR contains changes in dependencies, backward incompatibility, bug fixes, or new features. Labels increase the explicitness and help with the search (for example, finding all bug fixes becomes a matter of one click).
GitHub and GitLab allow you to use the entire MarkDown formatting arsenal (instructions are here for GH and here for GL). Proper formatting and structure will help to facilitate readability and simplify the understanding of many things. If the changes were applied to the GUI, then consider to supply screenshots of GUI state “before” and “after” the changes.
Remember that well-described PRs are helping with writing project documentation, in particular the “ChangeLog”, the “Release Notes”, the “Read Me”. For the first two, it is enough to indicate links to PRs, for “Read Me”, there is an ability to paste part of the content.
By composing a meaningful description of PR, you will help yourself and your colleagues not to waste time re-reading the code before the release of the new version — it would be narrowed to looking through the merged PRs since the time of the last release.
With this article, I tried to highlight that a meaningful and well-formatted PR description is a project document that is as valuable as a properly described ticket. If you have written the code for several hours, then it is also worth dedicating half an hour to summarize the work.
A habit to write good PR descriptions will greatly simplify the life for you and the project team, also saving a lot of time during reviews and discussions.
Here are some of the PRs for an open-source project that can serve as an example:
Here are some additional articles, that are related and useful:
Previously published at https://blog.maddevs.io/how-to-make-a-proper-description-for-a-pull-request-5e1cbe9fb374