# HG changeset patch # User Frederic Bastien # Date 1286392862 14400 # Node ID 17e9206af22ba110a13c7853887d7b24a7fb61a4 # Parent 56278ca00b4d633a0c995917193f54b7d17f2ddb updated the coding_review file and added our proposed politic diff -r 56278ca00b4d -r 17e9206af22b doc/v2_planning/code_review.txt --- 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 `_ @@ -54,6 +54,11 @@ - `Google Code `_ - 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.