visit
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: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:
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.
SLI-001, SLI-002 - Support for reading input from STDIN
SLI-002 - Bugfix for memory leak when input is supplied via STDIN
## Related tasks
- [SLI-001](//my.issue.tracker/project/SLI-001)
- [SLI-002](//my.issue.tracker/project/SLI-002)
## Depends on
- #3 (other opened PR in same repository)
- dependency#55 (other PR in the other related repository)
## 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:
- //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.
- //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.
## Changes
- Added dependency [ebnf](//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.
- 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.
## 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.
## 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.
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 and ). 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.Previously published at