-
Notifications
You must be signed in to change notification settings - Fork 1.2k
Improve PR description template #7706
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
Improve PR description template #7706
Conversation
.github/PULL_REQUEST_TEMPLATE.md
Outdated
| ## Potential risks | ||
| <!-- Assuming the worst case, what can be broken when deploying this change to production? --> | ||
| <!-- Any change is risky. Identify any potential risks involved in deploying this change to the production. | ||
| Pay special attention to potentially breaking backward compatibility, especially at the RPC and storage levels. --> |
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.
backwards and forwards, but maybe we don't have to call it out
.github/PULL_REQUEST_TEMPLATE.md
Outdated
|
|
||
| ## How did you test it? | ||
| <!-- How have you verified this change? Tested locally? Added a unit test? Checked in staging env? --> | ||
| <!-- Tested locally? Added a unit test? Added a functional test? Checked in CI? --> |
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.
lets make clear that this is checkboxes, not a questionary.
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.
You want me to list all possible options here? I guess there are too many of them, but I am not strictly against.
.github/PULL_REQUEST_TEMPLATE.md
Outdated
|
|
||
| ## Potential risks | ||
| <!-- Assuming the worst case, what can be broken when deploying this change to production? --> | ||
| <!-- Any change is risky. Identify any potential risks involved in deploying this change to the production. |
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.
Same here - optional.
If there are potential risks - worth shearing, if not - should be removed
|
|
||
| ## How did you test it? | ||
| <!-- How have you verified this change? Tested locally? Added a unit test? Checked in staging env? --> | ||
| - [ ] built |
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.
| - [ ] built | |
| - [ ] yolo | |
| - [ ] built |
(not a serious suggestion)
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.
"tested in production"
"my code doesn't need testing, it is flawless"
"checked by Chuck Norris"
There a bunch of good options.
|
|
||
| jobs: | ||
| strip-comments: | ||
| if: github.event.review.state == 'approved' |
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.
What makes this on first approval? That value is the overall approved status, not the type of review submitted?
What changed?
Removed useless sections. Improved others. Changed HTML comments to italic placeholders. Added GHA to validate placeholder removal (not blocking for now).
Why?
Team agreement.
How did you test it?