Please follow these guidelines when reviewing PRs.
-
If your code is ready to be reviewed and merged:
- Once you think your code is ready to merge, mark your draft pull request as “ready for review” or create a pull request if you don’t have a draft. The required reviewers will automatically be notified when you do this.
-
Else, if your code is ready to be reviewed, but not ready to be merged (for example, when it depends on another PR):
- Submit the pull request as draft.
- Leave it as draft.
- Add label
status:review.
- Assign reviewers manually.
-
You can post on the #code-review:matrix.mathesar.org for urgent PRs or to ask for optional review of draft PRs.
-
If the reviewer requests changes, please make the changes and re-request review.
-
Do not mark comments as resolved in the GitHub UI, let the reviewer do this.
-
If the reviewer has approved the PR but not merged it, feel free to merge the PR yourself.
-
Please avoid breaking changes to the API. If they absolutely can’t be avoided, please follow the process described here.
- Commit early, commit often.
- Write good commit messages.
- Create draft pull requests for in-progress work.
- Try to keep pull requests small if possible, since it makes review easier.
Anyone is welcome to review pull requests!
- Request changes if you want another look at the PR before it is merged.
- Resolve your own comments, do not resolve anyone else’s.
- If the branch needs to be updated before merging (because it’s out-of-date with the
master branch), do so, as long as the merge can be performed automatically. Otherwise, ask the Author to handle it.
- See Backend Code Review for guidelines specific to backend code.
¶ Maintainers
If your review is requested, it means that you are responsible for reviewing the pull request. If the PR is large or you think someone who is familiar with a specific part of the code would be helpful, feel free to request additional reviewers through the GitHub interface.
- Check for outstanding PRs at least once a day.
- We should be aiming to turn around PR reviews within 1-2 working days. This means that there should be movement on the PR every 1-2 days, a new review, code review fixes, repeat if needed, and then merge.
- If you approve the PR, merge it unless someone else has requested changes.
- If the person who has requested changes is unavailable, merge the PR anyway.
- Always merge using merge commits, never squash or rebase (the GitHub interface should disable squash and rebase, but check just in case).
- If the PR is from a community contributor and it only requires minor changes, feel free to make the changes yourself and merge them.
- We should be aiming to merge PRs in and create new issues for improvements rather than keeping PRs in review until every possible issue is fixed.
- Code review should be a fairly quick process. Reviewers should be focused on asking the right questions, not on doing research into the answers and suggesting them.
- e.g. if you’re wondering if the author considered a particular implication of a change they made, ask them that instead of doing research into all the implications yourself and informing the author of them.
- If you’d like to reconsider the architecture of a PR, create a draft issue for figuring that out rather than blocking the PR until you figure out the right architecture.
- When reviewing community contributed PRs, if it’s easier to make the changes yourself rather than describe the changes needed as a code review, just make the changes and merge the PR. You can explain what you did and thank the contributor for their work.
You can modify an in progress PR before merging, if necessary. If the PR is from a branch in the official Mathesar repository, just modify that branch. If it’s in a branch of a fork, it’s a bit more complicated. The smoothest way in that case is to
- Add the fork repo as a remote, locked to the appropriate branch and fetch:
git remote add -t $A_BRANCH_NAME -f $A_REMOTE_NAME $REPO_URL
Here,
$A_BRANCH_NAME is the 3rd party branch name.$A_REMOTE_NAME is chosen by the user; make it memorable.$REPO_URL is set to the url of the 3rd party repo in question.- Note that the
-f flag is not for ‘force’, but for ‘fetch’.
- Make changes in that branch.
- Add, commit, and push your changes (the branch should already have the correct remote set).
- Back in the official Mathesar repo, your changes should be shown. Review and merge as appropriate.
If (3) fails, it may be a permissions issue. In that case, you’ll have to make a new branch in the official Mathesar repo based off of the PR. To do that, follow the instructions here.
Some reading related to our process: