-
Notifications
You must be signed in to change notification settings - Fork 77
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Backport PR #2507 to release/v1.7 for add reviewer guideline #2508
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change | ||||
---|---|---|---|---|---|---|
@@ -0,0 +1,81 @@ | ||||||
# Guideline for the Pull Request reviewers | ||||||
|
||||||
This document serves as a guideline for those reviewing Pull Requests. | ||||||
|
||||||
This guideline is intended for internal developers. A separate guideline for external contributors is planned. | ||||||
|
||||||
## Purpose | ||||||
|
||||||
The purpose of this guideline is to clarify what reviewers should be aware of and to optimize communication, thereby facilitating efficient development of Vald. | ||||||
|
||||||
## Preparation | ||||||
|
||||||
This guideline assumes that the content of the Pull Request is appropriate. | ||||||
|
||||||
Consider splitting the Pull Request if the changes are extensive, have multiple intentions, or if the tests are insufficient. This makes it easier to review. | ||||||
|
||||||
Regarding the amount of change, opinions may vary, but in this project, we aim to keep the implementation of logic under 1000 lines, excluding automatically generated code such as Protocol Buffers. | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🚫 [documents testlint] <eslint.rules.write-good> reported by reviewdog 🐶 |
||||||
|
||||||
## Review request | ||||||
|
||||||
In this project, merging a Pull Request requires approval from two people. If you want a specific person to review, assign them explicitly; otherwise, assign random people. | ||||||
|
||||||
### Request notification | ||||||
|
||||||
People may notice requests through different means, but in this project, we prioritize the following methods of communication: | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🚫 [documents testlint] <eslint.rules.write-good> reported by reviewdog 🐶 |
||||||
|
||||||
1. If you are available to communicate with us via online meeting systems like Zoom, please feel free to notify us directly. | ||||||
1. Send a mention in some way. For example, we use [Slack](https://join.slack.com/t/vald-community/shared_invite/zt-db2ky9o4-R_9p2sVp8xRwztVa8gfnPA) and we have [X account](https://twitter.com/vdaas_vald). | ||||||
1. Email from GitHub is automatically sent when someone is assigned. | ||||||
|
||||||
If the reviewer does not notice the request, we follow the same priority for re-notifying. | ||||||
|
||||||
#### When Requested for Review | ||||||
|
||||||
Unless there are high-priority issues, you are in a meeting, or deeply focused on development, start the review immediately. | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🚫 [documents testlint] <eslint.rules.write-good> reported by reviewdog 🐶 |
||||||
|
||||||
## Approve condition | ||||||
|
||||||
- The content of the Pull Request is understood. | ||||||
- The content can be understood at a glance. | ||||||
- If it cannot be understood at a glance, request additional comments. | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🚫 [documents testlint] <eslint.rules.write-good> reported by reviewdog 🐶 |
||||||
- There are no deficiencies in the test cases. | ||||||
- The quality is suitable for release. | ||||||
- Compatibility | ||||||
- Security | ||||||
- Performance | ||||||
|
||||||
Regarding quality, while there is always room for improvement, if the quality is sufficient for release, it is acceptable to approve. If there is potential for improvement, suggest it in the comments, and let the implementer decide whether to address it in the same Pull Request or a separate one. | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🚫 [documents testlint] <eslint.rules.write-good> reported by reviewdog 🐶 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🚫 [documents testlint] <eslint.rules.write-good> reported by reviewdog 🐶 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🚫 [documents testlint] <eslint.rules.write-good> reported by reviewdog 🐶 |
||||||
|
||||||
### What Reviewers Should Do | ||||||
|
||||||
- If the title or description is unclear, ask questions before reviewing the code. | ||||||
- If any part of the code is not understood after a brief review, ask for clarification or additional comments instead of trying to understand it yourself. | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🚫 [documents testlint] <eslint.rules.write-good> reported by reviewdog 🐶 |
||||||
- Verify if the test cases are sufficient. | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🚫 [documents testlint] <eslint.rules.write-good> reported by reviewdog 🐶 |
||||||
- Approve immediately if there are no comments. | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🚫 [documents testlint] <eslint.rules.write-good> reported by reviewdog 🐶 |
||||||
- If interrupted, respond even if the review is incomplete. | ||||||
- Ignore other reviewers' comments. | ||||||
|
||||||
### What Reviewers Should Not Do | ||||||
|
||||||
- Try to decipher something that is not immediately understandable. | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🚫 [documents testlint] <eslint.rules.write-good> reported by reviewdog 🐶 |
||||||
- Close the browser or tab or start something else if it is not understood. | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🚫 [documents testlint] <eslint.rules.write-good> reported by reviewdog 🐶 |
||||||
- Attempt to verify the implementation in detail. | ||||||
|
||||||
### Communication Methods | ||||||
|
||||||
All communications do not need to be completed on GitHub. Using Slack or Zoom can make the review process more efficient. | ||||||
|
||||||
The following criteria often determine the choice of communication tool: | ||||||
|
||||||
- On GitHub Mainly used for specific questions or comments about the code. If the exchange is unsuitable for comments, use Slack or Zoom. | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Consider adding a comma after "On GitHub" for better readability. - On GitHub Mainly used for specific questions or comments about the code.
+ On GitHub, Mainly used for specific questions or comments about the code. Committable suggestion
Suggested change
|
||||||
- On Slack, used for brief exchanges. If the discussion is likely to be lengthy or complex, switch to Zoom. | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🚫 [documents testlint] <eslint.rules.write-good> reported by reviewdog 🐶 |
||||||
- On Zoom Used when more complex communication is necessary. | ||||||
|
||||||
Regardless of where communication takes place, ensure that the results are documented on GitHub or in the code as a record of the communication. | ||||||
|
||||||
## Effect measurement | ||||||
|
||||||
Once a month, a report is created at [Vald Issues](https://github.com/vdaas/vald/issues). | ||||||
|
||||||
This report includes the time taken from when a Pull Request is opened to when it is merged and a summary of this data. Regularly reflect on this information. | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🚫 [documents testlint] <eslint.rules.write-good> reported by reviewdog 🐶 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🚫 [documents testlint] <eslint.rules.write-good> reported by reviewdog 🐶 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🚫 [documents testlint] <eslint.rules.write-good> reported by reviewdog 🐶
"multiple" is wordy or unneeded (write-good)