Skip to content

Code Review

Jump to bottom
sternk edited this page Nov 24, 2014 · 1 revision

[If you are not familiar with git, have a look at http://git-scm.com/book]

Codereview guideline

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.

Roles

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 life of a pull-request

The usual workflow presents itself like this:

  1. The proposer has made some changes to the Hets 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)
  2. 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.
  • The pull request should be auto-mergeable into master
  1. 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.
  2. 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.
  3. The proposer responds to the comments, either by commenting herself, or by implementing the changes.
  4. Repeat steps 4. and 5. until every issue is resolved
  5. The reviewer signs off on the pull-request by commenting with a Thumbs up 👍 :+1:.
  6. 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.

Resolving merge conflicts with master

Preferably the branch should be rebased onto master before submitting a pull-request. Alternatively the last commit in the branch can be merge with master (see Merge-Flow below). In either case any merge conflicts will have to be resolved by hand.

Merge-Flow

  • Initial State: I'm done with my branch and want to create a pull-request

    • git merge master (ensure that master 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-commit perform your changes
      • git merge master

    • 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 master
Fixing the mess

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...

Review Notes

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 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?
    • Preferably no additional HLint warnings should appear
  • Does the proposal include new ghc packages or other dependencies? 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)
Clone this wiki locally