cerc-io/solidity
16
0
Fork 0
mirror of https://github.com/ethereum/solidity synced 2023-10-03 13:03:40 +00:00
Code Issues Actions Packages Projects Releases Wiki Activity

Merge pull request #13744 from ethereum/review-checklist-update-for-external-prs

Browse Source 
Update the review checklist to address common external PR problems
This commit is contained in:
Kamil Śliwak 2022-11-30 18:20:41 +01:00 committed by GitHub
commit c6ee18a507
No known key found for this signature in database
GPG Key ID: 4AEE18F83AFDEB23
1 changed files with 26 additions and 0 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

26
ReviewChecklist.md
View File

@ -7,6 +7,12 @@ and solve common issues before they are pointed out in the review.
It is also meant to serve as a final checklist for reviewers to go through before approving a PR. It is also meant to serve as a final checklist for reviewers to go through before approving a PR.
## Before You Submit a PR ## Before You Submit a PR
- [ ] **Do you have any other open PRs?**
Work on a PR is not done until it is merged or closed.
Our reviewing capacity is limited, so we require that external contributors work on **no more than one PR at a time**.
- If your PR is not getting reviewed, feel free to bring it to our attention on the [#solidity-dev](https://gitter.im/ethereum/solidity-dev) channel.
- Unless they were requested, we are going to close any excess PRs, leaving only the earliest one open.
You may reopen them, one at a time, when your current PR is done.
- [ ] **Is the issue ready to be worked on?** - [ ] **Is the issue ready to be worked on?**
- If the issue does not have a desirability label (`nice to have`, `should have`, - If the issue does not have a desirability label (`nice to have`, `should have`,
`must have eventually`, `must have`, `roadmap`) we have not yet decided whether to implement it. `must have eventually`, `must have`, `roadmap`) we have not yet decided whether to implement it.
@ -34,6 +40,26 @@ It is also meant to serve as a final checklist for reviewers to go through befor
- Review updated files before committing them. - Review updated files before committing them.
**Are expectations correct and do updated tests still serve their purpose?** **Are expectations correct and do updated tests still serve their purpose?**
## Abandoned PRs
- [ ] **Is the submitter still responsive?**
- If the PR had no activity from the submitter in the last 2 weeks (despite receiving reviews and our prompts) we consider it abandoned.
- [ ] **Is the abandoned PR easy to finish or relevant?**
- Apply the `takeover` label if the PR can be finished without significant effort or is something that actually needs to be done right now.
Otherwise close it.
It can still be taken over later or reopened by the submitter but until then we should not be getting sidetracked by it.
## Light Review
Before an in-depth review, it is recommended to give new PRs a quick, superficial review, which
is not meant to provide complete and detailed feedback, but instead give the submitter a rough idea
if the PR is even on the right track and let them solve the obvious problems on their own.
Light review should focus on the following three areas:
- [ ] **Are there any obvious mistakes?** Style issues, bad practices, easy to identify bugs, etc.
- [ ] **Is there anything missing?** Tests (of the right kind), documentation, etc. Does it address the whole issue?
- [ ] **Is it the right solution?** Are there better ways to do this? Is the change even necessary?
If the answers above are "Yes, Yes, No", thank the contributor for their effort and **close the PR**.
## Coding Style and Good Practices ## Coding Style and Good Practices
- [ ] Does the PR follow our [coding style](CODING_STYLE.md)? - [ ] Does the PR follow our [coding style](CODING_STYLE.md)?