Mercurial > pylearn
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