changeset 1246:14444845989a

add stuff to code review following the first meeting.
author Frederic Bastien <nouiz@nouiz.org>
date Thu, 23 Sep 2010 12:57:06 -0400
parents 808e38dce8d6
children 8dfe9d6e72f6
files doc/v2_planning/code_review.txt
diffstat 1 files changed, 42 insertions(+), 11 deletions(-) [+]
line wrap: on
line diff
--- a/doc/v2_planning/code_review.txt	Thu Sep 23 12:18:39 2010 -0400
+++ b/doc/v2_planning/code_review.txt	Thu Sep 23 12:57:06 2010 -0400
@@ -12,18 +12,19 @@
 
 - make a list of point to compare tools
 - review interresting projects
-- make a decission
+- 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
-- `Gerrit <http://code.google.com/p/gerrit/>`
-- `track PeerReviewPlugin <http://trac-hacks.org/wiki/PeerReviewPlugin>` Could be integrated with the current ticket system?
+- `rietveld <http://code.google.com/p/rietveld/>` Made by Guido van Rossum, seam basic and svn only
+- `Gerrit <http://code.google.com/p/gerrit/>`, git only
+- *`Review Board <http://www.reviewboard.org>`_ 
+- *`Code Striker <http://codestriker.sourceforge.net/>`, hg added? David told in May 2009 it can do it easily.
+- *`Code Review plugins in Redmine <http://www.redmine.org/boards/3/topics/9627>`
+- `track PeerReviewPlugin <http://trac-hacks.org/wiki/PeerReviewPlugin>` Could be integrated with the current ticket system?, not maintained, review code in general, not commit.
 - `feature request at assembla <http://feedback.assembla.com/forums/5433-feature-requests/suggestions/253297-add-a-code-review-tool-e-g-reviewboard->`
-- `Review Board <http://www.reviewboard.org>`_ 
-- `reviewboard <http://code.google.com/p/reviewboard/>`
-- `Code Striker <http://codestriker.sourceforge.net/>`
 - `JCR <http://jcodereview.sourceforge.net/>`
 
 What we could want from our code review
@@ -31,7 +32,13 @@
 
 - integrate with our ticket system?
     - Should we keep our current ticket system?
-- work with mercurial
+- 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
 ------------------
@@ -40,11 +47,35 @@
 - http://ostatic.com/blog/open-source-code-review-tools
 - http://en.wikipedia.org/wiki/Code_review
 
-Alternative to 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.
-- Test-Driven development
+- 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