Please follow these guidelines when reviewing PRs.
- 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.
- Please post in
#code-review:matrix.mathesar.org and tag the handles of the Mathesar engineering team (see Team) when you have a PR ready for review.
- 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.
- Check for outstanding PRs at least once a day.
- Review core team PRs within 1 work day, community PRs within 3 work days.
- 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.
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
$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: