--- a/doc/v2_planning/code_review.txt	Tue Dec 14 14:22:16 2010 -0500
+++ b/doc/v2_planning/code_review.txt	Tue Dec 14 14:53:48 2010 -0500
@@ -10,63 +10,67 @@
 TODO
 ----
 
-- Test the 2 proposed workflow (review board and github)
+- Test the 2 proposed workflows (review board and github)
+
+Reasons for code review
+-----------------------
 
-Reason for the code review
---------------------------
+- We want at least 2 people to read all code. That means we need a reviewer.
+- This helps to find better solutions to problems.
+- This helps to train people on our tools and framework.
+- This gives better code and less bugs in the end (everybody makes mistakes).
 
-- 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
-- This give better code and less bug in the end.
-    - Everybode make mistake...
+Proposed Policy
+---------------
+
+There are 2 proposals we want to test:
 
-Proposed Politic
-----------------
-
-Their is 2 proposal that we want to test:
-
-- GitHub fork/merge/merge request for new Pylearn
-    - When someone ask for a merge request, someone else make the review.
+- GitHub fork/merge/pull request for new Pylearn
+    - When someone asks for a pull request, someone else does the review.
+    - Everyone should work within their own Pylearn fork, and submit pull
+      requests to merge their code into the master Pylearn repository.
 - Review Board post-commit for Theano
-    - 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 it in many commits or start using it event if not fully tested.
-        - Reviewer should check that it is not enabled by default and when enabled it print a warning(once per job execution)
-    - We check all commit to Theano and Jobman.(Official tools)
-    - If the official review is not the right person for the task(gpu code,...)
-      He have the responsability to find the right persone (ask people in the lab, mailing-list, ...)
-    - The official reviewer should do:
-         - Review all code(see the check list) 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.
-         - We make a list of expert by domain of problem.(gpu, optimization, algo,...)
-    - For the test, the official reviewer work for one day.
-    - Later when we finish the test, maybe we will want to make that longer(1 week?)
-         - If some body break the build bot, it is him the reviewer for the next week/days
+    - For each commit message, if the author does not want this commit to be reviewed, say it in the message
+        - Useful for Official repo when we commit something that is disabled by default and we want to split it in many commits or start using it even if not fully tested.
+        - Reviewer should still check that it is not enabled by default and when enabled it prints a warning (once per job execution).
+    - We check all commits to Theano and Jobman (official tools).
+    - If the official reviewer is not the right person for the task (gpu code, ...)
+      he has the responsability to find someone else (ask people in the lab, mailing-list, ...).
+    - The official reviewer should:
+         - Review all code (see the checklist) and ask an expert when needed.
+         - Commit easy fixes and ask the original committer to review them
+           (non-trivial fixes are the responsibility of the original committer).
+    - The official reviewer is chosen among the Theano users in the lab with commit rights.
+    - In this test, we ask the official reviewer to review all commits from one day.
+    - We will set up a list of experts by domain (gpu, optimization, algo,...).
+    - After this test, we may want to make it longer (1 week?)
+         - If somebody breaks the build bot, he becomes the reviewer for the next week/days.
          - Maximum of one week by month.
-         - 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.
+         - If there is a big week or a rush that includes everybody in the lab, we can change more frequently.
+         - If a commit has problems, the original reviewer should follow-up on it.
 
-We should never be the official reviewer of our own code. When this happen, ask someone else to do it.
+Other general comments:
+- We should never be the official reviewer of our own code. When this happens,
+  ask the reviewer for the next day to take care of it.
+- Experimental code (not in Theano) may be tagged as not being reviewed (in
+  commit message).
 
-Check list for review
----------------------
+Checklist for review
+--------------------
 
-- Is their tests and do they test all case?
-    - Do test cover all case?
-- Is their documentation in the code file?
-    - Do this need doc in the html(public) documentation?
-- 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.
-- Check for conformence to our coding guideline
+- Are there tests and do they cover all cases?
+- Is there documentation in the code file?
+    - Should public (HTML) documentation be updated as well?
+- Are additions well integrated into our framework?
+- Is the code placed in the right files, and the right place in those files?
+- Try not to duplicate code.
+- Is the code clear and easy to understand?
+- Are there comments describing what is being done?
+- Answer potential questions by the committer (in the code). This can also help to train people.
+- Check for typos.
+- No debug code (print, breakpoints, ...).
+- If commit message says not to review, check that the code is disabled by default and that when enabled prints a warning.
+- Check for conformity to our coding guidelines.
 
 Some system that we checked
 ---------------------------