changeset 1314:17e9206af22b

updated the coding_review file and added our proposed politic
author Frederic Bastien <nouiz@nouiz.org>
date Wed, 06 Oct 2010 15:21:02 -0400
parents 56278ca00b4d
children f21693eecec7 3234913a3642
files doc/v2_planning/code_review.txt
diffstat 1 files changed, 36 insertions(+), 7 deletions(-) [+]
line wrap: on
line diff
--- a/doc/v2_planning/code_review.txt	Wed Oct 06 15:20:24 2010 -0400
+++ b/doc/v2_planning/code_review.txt	Wed Oct 06 15:21:02 2010 -0400
@@ -10,10 +10,10 @@
 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
+- Install Review Board and try it
+    - test review of merge
+    - how work with branch
+- Write our proposed politic
 
 Some system that we should check:
 ---------------------------------
@@ -30,7 +30,7 @@
     - 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)
+      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/>`_
@@ -54,6 +54,11 @@
 - `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
@@ -62,9 +67,11 @@
 - 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? patch? syntax highlight as vimdiff?
+- 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, ...)?
@@ -94,7 +101,7 @@
 
 - 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.
+- This help to train people on our tools and framework.
 
 Check list for review
 ---------------------
@@ -108,3 +115,25 @@
 - 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.