Reviewing¶
This document is a guide to coala’s review process.
Am I Good Enough to Do Code Review?¶
Yes, if you already fixed a newcomer issue.
Reviewing can help you understand the other side of the table and learn more about coala and python. When reviewing you will get to know new people, more code and it ultimately helps you to become a better coder than you could do on your own.
You can totally help us review source code. Especially try to review source
code of others and share what you have learnt with them. You can use acks and
unacks like everyone else and corobo
even allows you to set PRs to WIP.
Check the section below for more information.
Generally follow this process:
- Check if the change is helping us. Sometimes people propose changes that are only helpful for specific usecases but may break others. The concept has to be good. Consider engaging in a discussion on gitter if you are unsure!
- Check for automatic builds. Give the contributor hints on how he can resolve them.
- Review the actual code. Suggest improvements and simplifications.
Be sure to not value quantity over quality! Be transparent and polite: Explain why something has to be changed and don’t just “command” the coder to obey guidelines for no reason. Reviewing always involves saying someone that their code isn’t right, be very careful not to appear rude even if you don’t mean it! Bad reviews will scare away other contributors.
Note
Commits should have a good size, every logical change should be one commit. If there are multiple changes, those make multiple commits. Don’t propose to squash commits without a reason!
When reviewing, try to be thorough: if you don’t find any issues with a pull request, you likely missed something.
If you don’t find any issues with a Pull Request and acknowledge it, a senior member will look over it and perform the merge if everything is good.
Manual Review Process¶
The review process for coala is as follows:
Anyone can submit commits for review. These are submitted via Pull Requests on Github.
The Pull Request will be labeled with a
process
label:pending review
the commit has just been pushed and is awaiting reviewwip
the Pull Request has been marked as aWork in Progress
by the reviewers and has comments on it regarding how the commits shall be changedapproved
the commits have been reviewed by the developers and they are ready to be merged into the master branch
If you don’t have write access to coala, you can change the labels using
corobo mark wip <URL>
orcorobo mark pending <URL>
.The developers will acknowledge the commits by writing
- In case a member is reviewing it:
Looks good to me
better known asLGTM
in case the commit is ready.
- In case a maintainer is reviewing it:
ack commit_SHA
orcommit_SHA is ready
, in case the commit is ready, orunack commit_SHA
orcommit_SHA needs work
in case it is not ready yet and needs some more work orreack commit_SHA
in case the commit was acknowledged before, was rebased without conflicts and the rebase did not introduce logical problems.
Note
Only one acknowledgment is needed per commit i.e
ack commit_SHA
.If the commits are not linearly mergeable into master, rebase and go to step one.
All commits are acknowledged and fit linearly onto master. All continuous integration services (as described below) pass. A maintainer may leave the
@gitmate-bot ff
command to get the PR merged automatically.
Automated Review Process¶
It is only allowed to merge a pull request into master if all required
GitHub states are green. This includes the GitMate review as well as the
Continuous Integration systems.
Continuous integration is always done for the last commit on a pull request but should ideally pass for every commit.
For the Reviewers¶
- All the pull requests waiting to be reviewed can be found at : https://coala.io/review.
- Check the commit message.
- Read and try to understand the code. If something looks ineffective or bug prone, leave a comment. If in doubt, let the code-writer explain their reasoning until reviewers have understood the code.
- Generated code is not intended to be reviewed. Instead rather try to verify that the generation was done right. The commit message should expose that.
- Every commit is reviewed independently from the other commits.
- Tests should pass for each commit. If you suspect that tests might not pass and a commit is not checked by continuous integration, try running the tests locally.
- Check the surroundings. In many cases people forget to remove the import when removing the use of something or similar things. It is usually good to take a look at the whole file to see if it’s still consistent.
- Take a look at continuous integration results in the end even if they pass.
- Coverage must not fall.
- Be sure to assure that the tests cover all corner cases and validate the behaviour well. E.g. for bear tests just testing for a good and bad file is not sufficient. Writing Tests explains how tests should be written. Bears require special attention during testing. Testing Bears provides a guideline for how to test bears.
Note
While reviewing pull requests or patches for difficulty/low
issues,
make sure that the patch solves the issue and doesn’t create any
further issues.
You need to thoroughly review the code, i.e. understand the functionality of the code, check whether it is efficient or not, and leave critical comments. Otherwise, don’t review! We need human reviews to find the problems which can’t be found automatically.
As you perform your review of each commit, please make comments on the relevant lines of code in the GitHub pull request. After performing your review, please comment on the pull request directly as follows:
- If any commit passed review, make a comment that begins with “ack”, “reack”, or “ready” (all case-insensitive) and contains at least the first 6 characters of each passing commit hash delimited by spaces, commas, or forward slashes (the commit URLs from GitHub satisfy the commit hash requirements).
- If any commit failed to pass review, make a comment that begins with “unack” or “needs work” (all case-insensitive) and contains at least the first 6 characters of each passing commit hash delimited by spaces, commas, or forward slashes (the commit URLs from GitHub satisfy the commit hash requirements).
Note
GitMate only separates by spaces and commas. If you copy and paste the SHAs they sometimes contain tabs or other whitespace, be sure to remove those!
Example:
unack 14e3ae1 823e363 342700d
If you have a large number of commits to ack, you can easily generate a
list with git log --oneline master..
and write a message like this
example:
reack
a8cde5b Docs: Clarify that users may have pyvenv
5a05253 Docs: Change Developer Tutorials -> Resources
c3acb62 Docs: Create a set of notes for development setup
Rebased on top of changes that are not affected by documentation
changes.