|
| 1 | +## Reviewing PRs |
| 2 | + |
| 3 | +Reviewing PRs is an extremely important task in collaborative |
| 4 | +development. In fact, it is probably the task that requires the most |
| 5 | +time in the development, and it can be stressful for both the reviewer |
| 6 | +and the author. Remember that, as a PR Reviewer, you are guaranteeing |
| 7 | +that the changes work and integrate well with the rest of the |
| 8 | +repository, hence **you are responsible for the quality of the |
| 9 | +repository and its next version release**. If they don't integrate |
| 10 | +well, later PR reviewers might have to ask for broader changes than |
| 11 | +expected. |
| 12 | + |
| 13 | +There are many best practices to review code online, for |
| 14 | +instance [this medium blog post](https://medium.com/an-idea/the-code-review-guide-9e793edcd683), but |
| 15 | +here are some good rules of thumbs that we need to follow while |
| 16 | +reviewing PRs: |
| 17 | + |
| 18 | +- Be **respectful** to the PR authors and be clear in what you are |
| 19 | + asking/suggesting - remember that, like you, they are contributing |
| 20 | + their spare time and doing their best job! |
| 21 | + |
| 22 | +- If there is a *Draft PR*, you can comment on its development in the |
| 23 | + message board or making "Comment" reviews. Don't ask for changes, |
| 24 | + and especially, **don't approve the PR** |
| 25 | + |
| 26 | +- If the PR graduated *from Draft to full PR*, check that it follows the |
| 27 | + sections [Pull requests](#pull-requests) and [Style Guide](#style-guide) of these |
| 28 | + guidelines. If not, invite the author to do so before starting a |
| 29 | + review. |
| 30 | +- **Don't limit your review to the parts that are changed**. Look at |
| 31 | + the entire file, see if the changes fit well in it, and see if the |
| 32 | + changes are properly addressed everywhere in the code - in the |
| 33 | + documentation, in the tests, and in other functions. Sometimes the |
| 34 | + differences reported don't show the full impact of the PR in the |
| 35 | + repository! |
| 36 | +- If your want to make Pull Requests an educational process, invite |
| 37 | + the author of the PR to make changes before actually doing them |
| 38 | + yourself. Request changes via comments or in the message board or by |
| 39 | + checking out the PR locally, making changes and then submitting a PR |
| 40 | + to the author's branch. |
| 41 | +- If you decide to use the suggestion tool in reviews, or to start a |
| 42 | + PR to the branch under review, please alert the Project Manager. |
| 43 | + Bots might automatically assign you contribution types that will |
| 44 | + have to be removed (remember, your contribution in this case is |
| 45 | + "Reviewer"). Instead of starting a PR to the branch under review, |
| 46 | + think about opening a new PR with those modifications (unless they |
| 47 | + are needed to pass tests), and alert the Main Reviewer. In any case |
| 48 | + **don't commit directly to the branch under review**! |
| 49 | +- If you're reviewing documentation, build it locally with [`sphinx-build`](https://www.sphinx-doc.org/en/master/man/sphinx-build.html) command. |
| 50 | +- If you're asking for changes, **don't approve the PR**. Approve it |
| 51 | + only after everything was sufficiently addressed. Someone else might |
| 52 | + merge the PR in taking your word for granted. |
| 53 | +- If you are the main reviewer, and the last reviewer required to |
| 54 | + approve the PR, merge the PR! |
| 55 | + |
| 56 | +Before approving and/or merging PRs, be sure that: |
| 57 | + |
| 58 | +- All the tests in CircleCI/Azure pass without errors. |
| 59 | +- Prefereably, codecov checks pass as well. If they don't, discuss |
| 60 | + what to do. |
| 61 | +- The title describes the content of the PR clearly enough to be |
| 62 | + meaningful on its own - remember that it will appear in the version |
| 63 | + changelog! |
| 64 | +- The PR has the appropriate labels to trigger the appropriate version |
| 65 | + release and update the contributors table. |
| 66 | + |
| 67 | +### Main reviewer |
| 68 | + |
| 69 | +At `physiopy` we use the *Assignees* section of a PR to mark the |
| 70 | +**main reviewer** for that PR. The main reviewer is the primary person |
| 71 | +responsible **for the quality of the repository and its next version release**, |
| 72 | + as well as **for the behaviour of the other reviewers**. |
| 73 | + |
| 74 | +***The main reviewer takes care of the reviewing process of the PR, in particular:*** |
| 75 | + |
| 76 | +- Invites the reviewers to finish their review in a relatively |
| 77 | + short time. |
| 78 | + |
| 79 | +- Checks that all elements of this document were respected, |
| 80 | + especially the part about [Pull Requests](#pull-requests). |
| 81 | + |
| 82 | +- Invites other Reviewers to respect this document, especially |
| 83 | + the part about [reviews](#reviewing-prs), helps them in doing |
| 84 | + so, and checks that they do. |
| 85 | + |
| 86 | +- If a Reviewer keeps not respecting this document, flags them |
| 87 | + to the project manager. |
| 88 | + |
| 89 | +- Decides what to do in case of a coverage decrease (in |
| 90 | + *codecov/patch*). |
| 91 | + |
| 92 | +***In case of missing tests or updates to user documentation:*** |
| 93 | + |
| 94 | +- Asks for more documentation or tests before approving the |
| 95 | + PR, *or* |
| 96 | + |
| 97 | +- Checks that the appropriate issues have been opened to |
| 98 | + address the lack of documentation or tests (1 issue per item), *or* |
| 99 | + |
| 100 | +- Double-checks that the title is clear and the labels are correct to |
| 101 | + trigger an appropriate `auto` release - feel free to change them. |
| 102 | + |
| 103 | +- Main reviewer **Is the one that is going to merge the PR.** |
| 104 | + |
| 105 | +***After the PR has been merged and a new release has been triggered, checks that:*** |
| 106 | + |
| 107 | +- The documentation was updated correctly (if changed). |
| 108 | +- The Pypi version of the repository coincides with the new release (if changed). |
| 109 | +- New contributors or forms of contributions were correctly added in the README (if changed). |
0 commit comments