38 lines
1.2 KiB
Markdown
38 lines
1.2 KiB
Markdown
|
# Code review practices
|
||
|
|
|
||
|
|
## Write actionable comments
|
||
|
|
|
||
|
|
[Conventional comments](https://conventionalcomments.org/)
|
||
|
|
|
||
|
|
### Comment structure
|
||
|
|
|
||
|
|
```
|
||
|
|
label [decorators]: subject
|
||
|
|
|
||
|
|
discussion
|
||
|
|
|
||
|
|
[suggestion]
|
||
|
|
```
|
||
|
|
|
||
|
|
### Tags for blocking comments
|
||
|
|
|
||
|
|
- **problem**: Something is wrong with the code that means the PR should not be
|
||
|
|
merged.
|
||
|
|
- **question (blocking)**: Reviewer needs an explanation about part of the code
|
||
|
|
before they understand it well enough to approve.
|
||
|
|
- **cleanup**: Some vestigial code needs to be removed before merge.
|
||
|
|
- **typo**: Fix a typo.
|
||
|
|
|
||
|
|
### Non-blocking comments
|
||
|
|
|
||
|
|
All non-blocking tags need to be decorated as **non-blocking** to make it clear
|
||
|
|
to the PR author that the comment isn't blocking approval.
|
||
|
|
|
||
|
|
- **quibble (non-blocking)**: Something is wrong with the code but it's not
|
||
|
|
severe enough to block the PR from being merged.
|
||
|
|
- **polish (non-blocking)**: The code quality can be improved.
|
||
|
|
- **question (non-blocking)**: Reviewer needs an explanation about part of the
|
||
|
|
code before they understand it well enough to approve.
|
||
|
|
- **suggestion (non-blocking)**: A change of any kind that improves the
|
||
|
|
implementation or code quality, but isn't a blocking problem.
|