Mercurial > pylearn

annotate doc/v2_planning/code_review.txt @ 1253:826d78f0135f

Find changesets by keywords (author, files, the commit message), revision number or hash, or revset expression.
Prototype for "hooks" simpler than full control-flow rewrite.
author Pascal Lamblin <lamblinp@iro.umontreal.ca>
date Fri, 24 Sep 2010 01:46:12 -0400
parents 14444845989a
children 10113a1050ce
rev   line source
1209
5ff1d375fc33 added a first list of code review system.
Frederic Bastien <nouiz@nouiz.org>
parents:
diff changeset
1
5ff1d375fc33 added a first list of code review system.
Frederic Bastien <nouiz@nouiz.org>
parents:
diff changeset
2 Code Review
5ff1d375fc33 added a first list of code review system.
Frederic Bastien <nouiz@nouiz.org>
parents:
diff changeset
3 ===========
5ff1d375fc33 added a first list of code review system.
Frederic Bastien <nouiz@nouiz.org>
parents:
diff changeset
4
1235
a9b601119197 added project to review and other stuff in code_review.txt
Frederic Bastien <nouiz@nouiz.org>
parents: 1233
diff changeset
5 commity members
a9b601119197 added project to review and other stuff in code_review.txt
Frederic Bastien <nouiz@nouiz.org>
parents: 1233
diff changeset
6 ---------------
a9b601119197 added project to review and other stuff in code_review.txt
Frederic Bastien <nouiz@nouiz.org>
parents: 1233
diff changeset
7
a9b601119197 added project to review and other stuff in code_review.txt
Frederic Bastien <nouiz@nouiz.org>
parents: 1233
diff changeset
8 * Fred B.*, Pascal L., Olivier D., Arnaud B.
a9b601119197 added project to review and other stuff in code_review.txt
Frederic Bastien <nouiz@nouiz.org>
parents: 1233
diff changeset
9
a9b601119197 added project to review and other stuff in code_review.txt
Frederic Bastien <nouiz@nouiz.org>
parents: 1233
diff changeset
10 TODO
a9b601119197 added project to review and other stuff in code_review.txt
Frederic Bastien <nouiz@nouiz.org>
parents: 1233
diff changeset
11 ----
a9b601119197 added project to review and other stuff in code_review.txt
Frederic Bastien <nouiz@nouiz.org>
parents: 1233
diff changeset
12
a9b601119197 added project to review and other stuff in code_review.txt
Frederic Bastien <nouiz@nouiz.org>
parents: 1233
diff changeset
13 - make a list of point to compare tools
a9b601119197 added project to review and other stuff in code_review.txt
Frederic Bastien <nouiz@nouiz.org>
parents: 1233
diff changeset
14 - review interresting projects
1246
14444845989a add stuff to code review following the first meeting.
Frederic Bastien <nouiz@nouiz.org>
parents: 1235
diff changeset
15 - make a politic of review(who,what,what,how)
14444845989a add stuff to code review following the first meeting.
Frederic Bastien <nouiz@nouiz.org>
parents: 1235
diff changeset
16 - make a decission on projects
1235
a9b601119197 added project to review and other stuff in code_review.txt
Frederic Bastien <nouiz@nouiz.org>
parents: 1233
diff changeset
17
1209
5ff1d375fc33 added a first list of code review system.
Frederic Bastien <nouiz@nouiz.org>
parents:
diff changeset
18 Some system that we should check:
1235
a9b601119197 added project to review and other stuff in code_review.txt
Frederic Bastien <nouiz@nouiz.org>
parents: 1233
diff changeset
19 ---------------------------------
1209
5ff1d375fc33 added a first list of code review system.
Frederic Bastien <nouiz@nouiz.org>
parents:
diff changeset
20
1246
14444845989a add stuff to code review following the first meeting.
Frederic Bastien <nouiz@nouiz.org>
parents: 1235
diff changeset
21 - `rietveld <http://code.google.com/p/rietveld/>` Made by Guido van Rossum, seam basic and svn only
14444845989a add stuff to code review following the first meeting.
Frederic Bastien <nouiz@nouiz.org>
parents: 1235
diff changeset
22 - `Gerrit <http://code.google.com/p/gerrit/>`, git only
14444845989a add stuff to code review following the first meeting.
Frederic Bastien <nouiz@nouiz.org>
parents: 1235
diff changeset
23 - *`Review Board <http://www.reviewboard.org>`_
14444845989a add stuff to code review following the first meeting.
Frederic Bastien <nouiz@nouiz.org>
parents: 1235
diff changeset
24 - *`Code Striker <http://codestriker.sourceforge.net/>`, hg added? David told in May 2009 it can do it easily.
14444845989a add stuff to code review following the first meeting.
Frederic Bastien <nouiz@nouiz.org>
parents: 1235
diff changeset
25 - *`Code Review plugins in Redmine <http://www.redmine.org/boards/3/topics/9627>`
14444845989a add stuff to code review following the first meeting.
Frederic Bastien <nouiz@nouiz.org>
parents: 1235
diff changeset
26 - `track PeerReviewPlugin <http://trac-hacks.org/wiki/PeerReviewPlugin>` Could be integrated with the current ticket system?, not maintained, review code in general, not commit.
1209
5ff1d375fc33 added a first list of code review system.
Frederic Bastien <nouiz@nouiz.org>
parents:
diff changeset
27 - `feature request at assembla <http://feedback.assembla.com/forums/5433-feature-requests/suggestions/253297-add-a-code-review-tool-e-g-reviewboard->`
1235
a9b601119197 added project to review and other stuff in code_review.txt
Frederic Bastien <nouiz@nouiz.org>
parents: 1233
diff changeset
28 - `JCR <http://jcodereview.sourceforge.net/>`
1209
5ff1d375fc33 added a first list of code review system.
Frederic Bastien <nouiz@nouiz.org>
parents:
diff changeset
29
1235
a9b601119197 added project to review and other stuff in code_review.txt
Frederic Bastien <nouiz@nouiz.org>
parents: 1233
diff changeset
30 What we could want from our code review
a9b601119197 added project to review and other stuff in code_review.txt
Frederic Bastien <nouiz@nouiz.org>
parents: 1233
diff changeset
31 ---------------------------------------
1209
5ff1d375fc33 added a first list of code review system.
Frederic Bastien <nouiz@nouiz.org>
parents:
diff changeset
32
5ff1d375fc33 added a first list of code review system.
Frederic Bastien <nouiz@nouiz.org>
parents:
diff changeset
33 - integrate with our ticket system?
5ff1d375fc33 added a first list of code review system.
Frederic Bastien <nouiz@nouiz.org>
parents:
diff changeset
34 - Should we keep our current ticket system?
1246
14444845989a add stuff to code review following the first meeting.
Frederic Bastien <nouiz@nouiz.org>
parents: 1235
diff changeset
35 - work with mercurial, git?
14444845989a add stuff to code review following the first meeting.
Frederic Bastien <nouiz@nouiz.org>
parents: 1235
diff changeset
36 - check each commit of theano/pylearn
14444845989a add stuff to code review following the first meeting.
Frederic Bastien <nouiz@nouiz.org>
parents: 1235
diff changeset
37 - check experimental repository code when asked
14444845989a add stuff to code review following the first meeting.
Frederic Bastien <nouiz@nouiz.org>
parents: 1235
diff changeset
38 - how show diff? patch? syntax highlight as vimdiff?
14444845989a add stuff to code review following the first meeting.
Frederic Bastien <nouiz@nouiz.org>
parents: 1235
diff changeset
39 - 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.
14444845989a add stuff to code review following the first meeting.
Frederic Bastien <nouiz@nouiz.org>
parents: 1235
diff changeset
40 - Review should be done by everybody.
14444845989a add stuff to code review following the first meeting.
Frederic Bastien <nouiz@nouiz.org>
parents: 1235
diff changeset
41 - 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, ...)?
1235
a9b601119197 added project to review and other stuff in code_review.txt
Frederic Bastien <nouiz@nouiz.org>
parents: 1233
diff changeset
42
a9b601119197 added project to review and other stuff in code_review.txt
Frederic Bastien <nouiz@nouiz.org>
parents: 1233
diff changeset
43 Doc on code review
a9b601119197 added project to review and other stuff in code_review.txt
Frederic Bastien <nouiz@nouiz.org>
parents: 1233
diff changeset
44 ------------------
a9b601119197 added project to review and other stuff in code_review.txt
Frederic Bastien <nouiz@nouiz.org>
parents: 1233
diff changeset
45
a9b601119197 added project to review and other stuff in code_review.txt
Frederic Bastien <nouiz@nouiz.org>
parents: 1233
diff changeset
46 - http://code.google.com/p/support/wiki/CodeReviews
a9b601119197 added project to review and other stuff in code_review.txt
Frederic Bastien <nouiz@nouiz.org>
parents: 1233
diff changeset
47 - http://ostatic.com/blog/open-source-code-review-tools
a9b601119197 added project to review and other stuff in code_review.txt
Frederic Bastien <nouiz@nouiz.org>
parents: 1233
diff changeset
48 - http://en.wikipedia.org/wiki/Code_review
a9b601119197 added project to review and other stuff in code_review.txt
Frederic Bastien <nouiz@nouiz.org>
parents: 1233
diff changeset
49
1246
14444845989a add stuff to code review following the first meeting.
Frederic Bastien <nouiz@nouiz.org>
parents: 1235
diff changeset
50 Type of code review
14444845989a add stuff to code review following the first meeting.
Frederic Bastien <nouiz@nouiz.org>
parents: 1235
diff changeset
51 -------------------
1235
a9b601119197 added project to review and other stuff in code_review.txt
Frederic Bastien <nouiz@nouiz.org>
parents: 1233
diff changeset
52
1246
14444845989a add stuff to code review following the first meeting.
Frederic Bastien <nouiz@nouiz.org>
parents: 1235
diff changeset
53 - Formal review - Many person review together each line of the program.
1235
a9b601119197 added project to review and other stuff in code_review.txt
Frederic Bastien <nouiz@nouiz.org>
parents: 1233
diff changeset
54 - Over-the-shoulder – One developer looks over the author's shoulder as the latter walks through the code.
a9b601119197 added project to review and other stuff in code_review.txt
Frederic Bastien <nouiz@nouiz.org>
parents: 1233
diff changeset
55 - Email pass-around – Source code management system emails code to reviewers automatically after checkin is made.
a9b601119197 added project to review and other stuff in code_review.txt
Frederic Bastien <nouiz@nouiz.org>
parents: 1233
diff changeset
56 - Pair Programming – Two authors develop code together at the same workstation, such is common in Extreme Programming.
a9b601119197 added project to review and other stuff in code_review.txt
Frederic Bastien <nouiz@nouiz.org>
parents: 1233
diff changeset
57 - Tool-assisted code review – Authors and reviewers use specialized tools designed for peer code review.
1246
14444845989a add stuff to code review following the first meeting.
Frederic Bastien <nouiz@nouiz.org>
parents: 1235
diff changeset
58 - Alternative: Test-Driven development
14444845989a add stuff to code review following the first meeting.
Frederic Bastien <nouiz@nouiz.org>
parents: 1235
diff changeset
59 - Automatic review: use tool as pylint, pyflakes, pychecker. Don't check everything.
14444845989a add stuff to code review following the first meeting.
Frederic Bastien <nouiz@nouiz.org>
parents: 1235
diff changeset
60
14444845989a add stuff to code review following the first meeting.
Frederic Bastien <nouiz@nouiz.org>
parents: 1235
diff changeset
61 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.
14444845989a add stuff to code review following the first meeting.
Frederic Bastien <nouiz@nouiz.org>
parents: 1235
diff changeset
62
14444845989a add stuff to code review following the first meeting.
Frederic Bastien <nouiz@nouiz.org>
parents: 1235
diff changeset
63 Reason for the code review
14444845989a add stuff to code review following the first meeting.
Frederic Bastien <nouiz@nouiz.org>
parents: 1235
diff changeset
64 --------------------------
14444845989a add stuff to code review following the first meeting.
Frederic Bastien <nouiz@nouiz.org>
parents: 1235
diff changeset
65
14444845989a add stuff to code review following the first meeting.
Frederic Bastien <nouiz@nouiz.org>
parents: 1235
diff changeset
66 - We want at least 2 people to read all code. That mean we need a reviewer
14444845989a add stuff to code review following the first meeting.
Frederic Bastien <nouiz@nouiz.org>
parents: 1235
diff changeset
67 - This help to find better solution to problem
14444845989a add stuff to code review following the first meeting.
Frederic Bastien <nouiz@nouiz.org>
parents: 1235
diff changeset
68 - This help to train people on our tools ans framework.
14444845989a add stuff to code review following the first meeting.
Frederic Bastien <nouiz@nouiz.org>
parents: 1235
diff changeset
69
14444845989a add stuff to code review following the first meeting.
Frederic Bastien <nouiz@nouiz.org>
parents: 1235
diff changeset
70 Check list for review
14444845989a add stuff to code review following the first meeting.
Frederic Bastien <nouiz@nouiz.org>
parents: 1235
diff changeset
71 ---------------------
14444845989a add stuff to code review following the first meeting.
Frederic Bastien <nouiz@nouiz.org>
parents: 1235
diff changeset
72
14444845989a add stuff to code review following the first meeting.
Frederic Bastien <nouiz@nouiz.org>
parents: 1235
diff changeset
73 - Is their tests and do they test all case?
14444845989a add stuff to code review following the first meeting.
Frederic Bastien <nouiz@nouiz.org>
parents: 1235
diff changeset
74 - Is their documentation in the file?
14444845989a add stuff to code review following the first meeting.
Frederic Bastien <nouiz@nouiz.org>
parents: 1235
diff changeset
75 - Do this need doc in the html doc?
14444845989a add stuff to code review following the first meeting.
Frederic Bastien <nouiz@nouiz.org>
parents: 1235
diff changeset
76 - Is the addition well integrated into our framework
14444845989a add stuff to code review following the first meeting.
Frederic Bastien <nouiz@nouiz.org>
parents: 1235
diff changeset
77 - Is the code well placed in the right files and right place in them?
14444845989a add stuff to code review following the first meeting.
Frederic Bastien <nouiz@nouiz.org>
parents: 1235
diff changeset
78 - Try to don't duplicate code
14444845989a add stuff to code review following the first meeting.
Frederic Bastien <nouiz@nouiz.org>
parents: 1235
diff changeset
79 - Is the code clear/comprehensible
14444845989a add stuff to code review following the first meeting.
Frederic Bastien <nouiz@nouiz.org>
parents: 1235
diff changeset
80 - Are the comment describing what is being done?
14444845989a add stuff to code review following the first meeting.
Frederic Bastien <nouiz@nouiz.org>
parents: 1235
diff changeset
81 - Answer question by de commiter, this can also serve to train people