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: 1. 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! 2. Check for automatic builds. Give the contributor hints on how he can resolve them. 3. 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: 1. Anyone can submit commits for review. These are submitted via Pull Requests on Github. 2. The Pull Request will be labeled with a ``process`` label: - ``pending review`` the commit has just been pushed and is awaiting review - ``wip`` the Pull Request has been marked as a ``Work in Progress`` by the reviewers and has comments on it regarding how the commits shall be changed - ``approved`` 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 `` or ``corobo mark pending ``. 3. The developers will acknowledge the commits by writing * In case a member is reviewing it: - ``Looks good to me`` better known as ``LGTM`` in case the commit is ready. * In case a maintainer is reviewing it: - ``ack commit_SHA`` or ``commit_SHA is ready``, in case the commit is ready, or - ``unack commit_SHA`` or ``commit_SHA needs work`` in case it is not ready yet and needs some more work or - ``reack 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``. 4. If the commits are not linearly mergeable into master, rebase and go to step one. 5. 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: .. code-block:: none 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: .. code-block:: none 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.