Code Review
===========

commity members
---------------

* Fred B.*, Pascal L., Olivier D., Arnaud B.

TODO
----

- make a list of point to compare tools
- review interresting projects
- make a politic of review(who,what,what,how)
- make a decission on projects

Some system that we should check:
---------------------------------

- `rietveld <http://code.google.com/p/rietveld/>`_
    - Made by Guido van Rossum, seam basic and svn only
    - No, not enough features

- `Gerrit <http://code.google.com/p/gerrit/>`_
    - git only
    - No

- `Review Board <http://www.reviewboard.org>`_
    - Interesting, but some questions remain (how well it integrates with hg,
      notably)
    - Some advantages over Google code (comment on multi-line chunks, list of
      unreviewed commits)
    - Fred will install it so we can test it more thoroughly

- `Code Striker <http://codestriker.sourceforge.net/>`_
    - hg added? David told in May 2009 it can do it easily.
    - Seems less interesting than Review Board
    - No

- `Code Review plugins in Redmine <http://www.redmine.org/boards/3/topics/9627>`_
    - No

- `track PeerReviewPlugin <http://trac-hacks.org/wiki/PeerReviewPlugin>`_
    - Could be integrated with the current ticket system?, not maintained, review code in general, not commit.
    - No

- `feature request at assembla <http://feedback.assembla.com/forums/5433-feature-requests/suggestions/253297-add-a-code-review-tool-e-g-reviewboard->`_
    - No (we will not wait until the feature is added...)

- `JCR <http://jcodereview.sourceforge.net/>`_
    - No

- `Google Code <http://code.google.com/>`_
    - Test bench with a clone of Theano at
      http://code.google.com/p/theanoclone/
    - Maybe

What we could want from our code review
---------------------------------------

- integrate with our ticket system?
    - Should we keep our current ticket system?
- work with mercurial, git?
- check each commit of theano/pylearn
- check experimental repository code when asked
- how show diff? patch? syntax highlight as vimdiff?
- If we commit something that is disabled by default and not fully working, we can say it in the commit message to have a faster review(only check that by default it is disabled). Then we should say in the commit message when it is ready for a full review.
- Review should be done by everybody.
- Who choose the reviewer(random, commiter)? pool of reviewers? pool level 1,2,3 where 1 is everybody with commit right. pool for specific topic(gpu, ML algo, ...)?

Doc on code review
------------------

- http://code.google.com/p/support/wiki/CodeReviews
- http://ostatic.com/blog/open-source-code-review-tools
- http://en.wikipedia.org/wiki/Code_review

Type of code review
-------------------

- Formal review - Many person review together each line of the program.
- Over-the-shoulder – One developer looks over the author's shoulder as the latter walks through the code.
- Email pass-around – Source code management system emails code to reviewers automatically after checkin is made.
- Pair Programming – Two authors develop code together at the same workstation, such is common in Extreme Programming.
- Tool-assisted code review – Authors and reviewers use specialized tools designed for peer code review.
- Alternative: Test-Driven development
- Automatic review: use tool as pylint, pyflakes, pychecker. Don't check everything.

We seam to do Over-the-shoulder, email and variant of pair programming from time to time. Some people read rapidly the commit of Theano and Pylearn.

Reason for the code review
--------------------------

- We want at least 2 people to read all code. That mean we need a reviewer
- This help to find better solution to problem
- This help to train people on our tools ans framework.

Check list for review
---------------------

- Is their tests and do they test all case?
- Is their documentation in the file?
    - Do this need doc in the html doc?
- Is the addition well integrated into our framework
- Is the code well placed in the right files and right place in them?
- Try to don't duplicate code
- Is the code clear/comprehensible
- Are the comment describing what is being done?
- Answer question by de commiter, this can also serve to train people