Code Review
[If you are not familiar with git, have a look at http://git-scm.com/book]
[For our ruby style guidelines, see https://github.com/ontohub/ruby-style-guide]
This is just intended as a guideline for code reviews (during pull-request). There may be specific cases in which a different workflow is used. But in the general case we should conform to this guide.
During the course of a pull-request there are two important roles, which are represented by two different persons:
- the proposer is the person who proposes the pull-request, which is usually the person who participated in creating the majority of the code that is proposed to be merged.
- the reviewer is the central person which reviews the pull-request and signs off on it. Every other developer is encouraged to read the pull-request and comment on it, if there is something noteworthy.
The usual workflow presents itself like this:
- The proposer has made some changes to the ontohub codebase
and pushed these changes into a specific branch.
(either an issue-branch, which contains the github-issue-id, or a hotfix/general branch which is expressively named) - The proposer creates a pull-request.
The description should contain some information as to what this pull-request is supposed to solve. In the case of an issue-related pull-request the proposer shall reference the issue in the description.
- Part of the pull-request should be an up-to-date merge of staging (see staging-merge below).
- The reviewer starts the reviewing process.
Some notes on the specific parts the reviewer should pay attention to are outlined in the Review Notes section below. - The reviewer comments on the pull-request by suggesting possible changes and changes which would need to be done for this pull-request to be accepted. The reviewer should refrain from committing changes by herself.
- The proposer responds to the comments, either by commenting herself, or by implementing the changes.
- Repeat steps 4. and 5. until every issue is resolved
- The reviewer signs off on the pull-request by commenting
with a Thumbs up 👍
:+1:. - The proposer merges the pull-request (and closes, if applicable, the corresponding issue). If the branch isn't needed anymore the proposer shall delete it as soon as possible.
As the title implies there should be exactly one merge of staging into the issue-branch. This merge should be the last commit in the branch (also called branch-HEAD in git).
The reason for that is, that multiple merges unnecessarily clutter the branch with interim commits, which may have conflicts on them. But more importantly the make it (nearly) impossible to interactively rebase the branch in order to merge or split commits or fix small errors without producing another commit.
So that is why there should only be one merge of staging per issue-branch.
It also needs to be the last commit to make it easy to rebase or to update the merge:
Updating the merge is possible by removing the merge-commit (through reset) and then performing a new merge. This is basically the same way for rebasing: removing the merge-commit, performing the rebase action and then performing a new merge.
But what if i need some new changes of staging in order to complete the work on my branch?
We use rebase for that. If you'll need some staging for your branch you can just rebase your branch via staging.
- Initial State: I'm done with my branch and want to create a pull-request
-
git merge staging(ensure that staging is up-to-date first) -
A Pull-Request comment requires you to change a commit:
-
git reset --hard HEAD^(will delete the changes the last commit (the merge) made to your branch) -
git rebase -i parent-of-some-commitperform your changes git merge staging
-
-
You want to be up-to-date with the real staging:
-
git reset --hard HEAD^(will delete the changes the last commit (the merge) made to your branch) git merge staging
-
-
It might happen that you've created your final-staging merge and then performed some additional commits without noticing. That is not really a bad thing. If it is really necessary we will also accept this. But generally you should try to remove the merge-commit:
git rebase -p --onto <merge-commit-sha>^ <merge-commit-sha>
If this doesn't work somehow you could just reset back to before the merge and then cherry-pick the new commits. Or you just accept it...
- Initial State: I'm working on my branch, but need some new changes which are part of staging
-
git rebase staging(ensure that staging is up-to-date first)- Perform possible conflict resolution
This is about the questions a reviewer might ask herself when reviewing a pull-request.
Please feel free to alter and extend this list.
- Does the proposal include tests?
- Are specs (preferable) used instead of test/unit tests?
- Should the proposal contain system (integration or high-level) tests?
- If it should, does it contain these tests?
- Are all tests passing? (At least on travis, preferably check on your own system too)
- Does the code solve the actual problem?
- Is there a better (more concise) way to express code excerpts?
- Are there useful optimizations/refactorings which could be done right now?
- Does the code conform to our style guidelines?
- Should we adjust our style guidelines based on something we can learn from this code?
- Does the proposal include new gems? If so, why? (is there a good reason?).
- Can a specific change be generalized as such that other parts of the codebase can benefit?
- Is a rather complex method documented? (through code-comments)
- Do you have problems understanding part of the code? If so, ask, or propose simplifying the code (if this is at all possible)