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

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

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

TODO
----

- Install Review Board and try it
    - test review of merge
    - how work with branch
- Write our proposed politic

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, more esthetics, handle many repo, keep assemble easily)
    - 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/
    - post-commit
    - no list of not reviewed commit
    - no python syntax highlight
    - weird comment by line
    - diff of merge seam bugged
    - 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?
    - how branch/merge are handled
- check each commit of theano/pylearn
- check experimental repository code when asked
- how show diff? diff as vimdiff? patch? 
- syntax highlight
- 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 and 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
- Check for typo
- No debug code(print, breakpoint,...)
- If commit message tell to don't review, check that the code is disabled by default and that when enabled print a warning.

Proposed Politic
----------------

- For each commit message, if the author don't want this commit to be reviewed, tell it in the message
   - Usefull for experimental repository, not Theano
   - Usefull for Official repo when we commit something that is disabled by default and we want to split in many commits or start using event if not fully tested.
   - Reviewer should check that the check is not enabled by default and when enabled should print a warning.
- We check all commit to Theano, Pylearn and Jobman.(Official tools)
- We check experimental repos when asked.
- One official reviewer per week.
    - He review all code and ask expert when needed.
    - Should check the check list again all review.
    - We choose the reviewer in the theano user of the lab with commit right.
    - On fait une list d'expert par demain de problem(gpu, optimization, algo,...)
    - If some body break the build bot, it is him the reviewer for the next week
    - Maximum of one week by mount.
    - If their is big week or during rush that include every body of the lab, we can change more frequently.
    - If a commit have problem, it is the original reviewer that should make the follow up.