Mercurial > pylearn

annotate doc/v2_planning/code_review.txt @ 1310:f5e9c00a67d7

Find changesets by keywords (author, files, the commit message), revision number or hash, or revset expression.
fix rst problem.
author Frederic Bastien <nouiz@nouiz.org>
date Tue, 05 Oct 2010 12:31:36 -0400
parents 10113a1050ce
children 17e9206af22b
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
1263
10113a1050ce More RST
Pascal Lamblin <lamblinp@iro.umontreal.ca>
parents: 1246
diff changeset
21 - `rietveld <http://code.google.com/p/rietveld/>`_
10113a1050ce More RST
Pascal Lamblin <lamblinp@iro.umontreal.ca>
parents: 1246
diff changeset
22 - Made by Guido van Rossum, seam basic and svn only
10113a1050ce More RST
Pascal Lamblin <lamblinp@iro.umontreal.ca>
parents: 1246
diff changeset
23 - No, not enough features
10113a1050ce More RST
Pascal Lamblin <lamblinp@iro.umontreal.ca>
parents: 1246
diff changeset
24
10113a1050ce More RST
Pascal Lamblin <lamblinp@iro.umontreal.ca>
parents: 1246
diff changeset
25 - `Gerrit <http://code.google.com/p/gerrit/>`_
10113a1050ce More RST
Pascal Lamblin <lamblinp@iro.umontreal.ca>
parents: 1246
diff changeset
26 - git only
10113a1050ce More RST
Pascal Lamblin <lamblinp@iro.umontreal.ca>
parents: 1246
diff changeset
27 - No
10113a1050ce More RST
Pascal Lamblin <lamblinp@iro.umontreal.ca>
parents: 1246
diff changeset
28
10113a1050ce More RST
Pascal Lamblin <lamblinp@iro.umontreal.ca>
parents: 1246
diff changeset
29 - `Review Board <http://www.reviewboard.org>`_
10113a1050ce More RST
Pascal Lamblin <lamblinp@iro.umontreal.ca>
parents: 1246
diff changeset
30 - Interesting, but some questions remain (how well it integrates with hg,
10113a1050ce More RST
Pascal Lamblin <lamblinp@iro.umontreal.ca>
parents: 1246
diff changeset
31 notably)
10113a1050ce More RST
Pascal Lamblin <lamblinp@iro.umontreal.ca>
parents: 1246
diff changeset
32 - Some advantages over Google code (comment on multi-line chunks, list of
10113a1050ce More RST
Pascal Lamblin <lamblinp@iro.umontreal.ca>
parents: 1246
diff changeset
33 unreviewed commits)
10113a1050ce More RST
Pascal Lamblin <lamblinp@iro.umontreal.ca>
parents: 1246
diff changeset
34 - Fred will install it so we can test it more thoroughly
10113a1050ce More RST
Pascal Lamblin <lamblinp@iro.umontreal.ca>
parents: 1246
diff changeset
35
10113a1050ce More RST
Pascal Lamblin <lamblinp@iro.umontreal.ca>
parents: 1246
diff changeset
36 - `Code Striker <http://codestriker.sourceforge.net/>`_
10113a1050ce More RST
Pascal Lamblin <lamblinp@iro.umontreal.ca>
parents: 1246
diff changeset
37 - hg added? David told in May 2009 it can do it easily.
10113a1050ce More RST
Pascal Lamblin <lamblinp@iro.umontreal.ca>
parents: 1246
diff changeset
38 - Seems less interesting than Review Board
10113a1050ce More RST
Pascal Lamblin <lamblinp@iro.umontreal.ca>
parents: 1246
diff changeset
39 - No
10113a1050ce More RST
Pascal Lamblin <lamblinp@iro.umontreal.ca>
parents: 1246
diff changeset
40
10113a1050ce More RST
Pascal Lamblin <lamblinp@iro.umontreal.ca>
parents: 1246
diff changeset
41 - `Code Review plugins in Redmine <http://www.redmine.org/boards/3/topics/9627>`_
10113a1050ce More RST
Pascal Lamblin <lamblinp@iro.umontreal.ca>
parents: 1246
diff changeset
42 - No
10113a1050ce More RST
Pascal Lamblin <lamblinp@iro.umontreal.ca>
parents: 1246
diff changeset
43
10113a1050ce More RST
Pascal Lamblin <lamblinp@iro.umontreal.ca>
parents: 1246
diff changeset
44 - `track PeerReviewPlugin <http://trac-hacks.org/wiki/PeerReviewPlugin>`_
10113a1050ce More RST
Pascal Lamblin <lamblinp@iro.umontreal.ca>
parents: 1246
diff changeset
45 - Could be integrated with the current ticket system?, not maintained, review code in general, not commit.
10113a1050ce More RST
Pascal Lamblin <lamblinp@iro.umontreal.ca>
parents: 1246
diff changeset
46 - No
10113a1050ce More RST
Pascal Lamblin <lamblinp@iro.umontreal.ca>
parents: 1246
diff changeset
47
10113a1050ce More RST
Pascal Lamblin <lamblinp@iro.umontreal.ca>
parents: 1246
diff changeset
48 - `feature request at assembla <http://feedback.assembla.com/forums/5433-feature-requests/suggestions/253297-add-a-code-review-tool-e-g-reviewboard->`_
10113a1050ce More RST
Pascal Lamblin <lamblinp@iro.umontreal.ca>
parents: 1246
diff changeset
49 - No (we will not wait until the feature is added...)
10113a1050ce More RST
Pascal Lamblin <lamblinp@iro.umontreal.ca>
parents: 1246
diff changeset
50
10113a1050ce More RST
Pascal Lamblin <lamblinp@iro.umontreal.ca>
parents: 1246
diff changeset
51 - `JCR <http://jcodereview.sourceforge.net/>`_
10113a1050ce More RST
Pascal Lamblin <lamblinp@iro.umontreal.ca>
parents: 1246
diff changeset
52 - No
10113a1050ce More RST
Pascal Lamblin <lamblinp@iro.umontreal.ca>
parents: 1246
diff changeset
53
10113a1050ce More RST
Pascal Lamblin <lamblinp@iro.umontreal.ca>
parents: 1246
diff changeset
54 - `Google Code <http://code.google.com/>`_
10113a1050ce More RST
Pascal Lamblin <lamblinp@iro.umontreal.ca>
parents: 1246
diff changeset
55 - Test bench with a clone of Theano at
10113a1050ce More RST
Pascal Lamblin <lamblinp@iro.umontreal.ca>
parents: 1246
diff changeset
56 http://code.google.com/p/theanoclone/
10113a1050ce More RST
Pascal Lamblin <lamblinp@iro.umontreal.ca>
parents: 1246
diff changeset
57 - Maybe
1209
5ff1d375fc33 added a first list of code review system.
Frederic Bastien <nouiz@nouiz.org>
parents:
diff changeset
58
1235
a9b601119197 added project to review and other stuff in code_review.txt
Frederic Bastien <nouiz@nouiz.org>
parents: 1233
diff changeset
59 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
60 ---------------------------------------
1209
5ff1d375fc33 added a first list of code review system.
Frederic Bastien <nouiz@nouiz.org>
parents:
diff changeset
61
5ff1d375fc33 added a first list of code review system.
Frederic Bastien <nouiz@nouiz.org>
parents:
diff changeset
62 - integrate with our ticket system?
5ff1d375fc33 added a first list of code review system.
Frederic Bastien <nouiz@nouiz.org>
parents:
diff changeset
63 - 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
64 - work with mercurial, git?
14444845989a add stuff to code review following the first meeting.
Frederic Bastien <nouiz@nouiz.org>
parents: 1235
diff changeset
65 - 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
66 - 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
67 - 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
68 - 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
69 - 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
70 - 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
71
a9b601119197 added project to review and other stuff in code_review.txt
Frederic Bastien <nouiz@nouiz.org>
parents: 1233
diff changeset
72 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
73 ------------------
a9b601119197 added project to review and other stuff in code_review.txt
Frederic Bastien <nouiz@nouiz.org>
parents: 1233
diff changeset
74
a9b601119197 added project to review and other stuff in code_review.txt
Frederic Bastien <nouiz@nouiz.org>
parents: 1233
diff changeset
75 - 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
76 - 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
77 - 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
78
1246
14444845989a add stuff to code review following the first meeting.
Frederic Bastien <nouiz@nouiz.org>
parents: 1235
diff changeset
79 Type of code review
14444845989a add stuff to code review following the first meeting.
Frederic Bastien <nouiz@nouiz.org>
parents: 1235
diff changeset
80 -------------------
1235
a9b601119197 added project to review and other stuff in code_review.txt
Frederic Bastien <nouiz@nouiz.org>
parents: 1233
diff changeset
81
1246
14444845989a add stuff to code review following the first meeting.
Frederic Bastien <nouiz@nouiz.org>
parents: 1235
diff changeset
82 - 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
83 - 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
84 - 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
85 - 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
86 - 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
87 - Alternative: Test-Driven development
14444845989a add stuff to code review following the first meeting.
Frederic Bastien <nouiz@nouiz.org>
parents: 1235
diff changeset
88 - 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
89
14444845989a add stuff to code review following the first meeting.
Frederic Bastien <nouiz@nouiz.org>
parents: 1235
diff changeset
90 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
91
14444845989a add stuff to code review following the first meeting.
Frederic Bastien <nouiz@nouiz.org>
parents: 1235
diff changeset
92 Reason for the code review
14444845989a add stuff to code review following the first meeting.
Frederic Bastien <nouiz@nouiz.org>
parents: 1235
diff changeset
93 --------------------------
14444845989a add stuff to code review following the first meeting.
Frederic Bastien <nouiz@nouiz.org>
parents: 1235
diff changeset
94
14444845989a add stuff to code review following the first meeting.
Frederic Bastien <nouiz@nouiz.org>
parents: 1235
diff changeset
95 - 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
96 - 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
97 - 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
98
14444845989a add stuff to code review following the first meeting.
Frederic Bastien <nouiz@nouiz.org>
parents: 1235
diff changeset
99 Check list for review
14444845989a add stuff to code review following the first meeting.
Frederic Bastien <nouiz@nouiz.org>
parents: 1235
diff changeset
100 ---------------------
14444845989a add stuff to code review following the first meeting.
Frederic Bastien <nouiz@nouiz.org>
parents: 1235
diff changeset
101
14444845989a add stuff to code review following the first meeting.
Frederic Bastien <nouiz@nouiz.org>
parents: 1235
diff changeset
102 - 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
103 - 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
104 - 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
105 - 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
106 - 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
107 - 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
108 - Is the code clear/comprehensible
14444845989a add stuff to code review following the first meeting.
Frederic Bastien <nouiz@nouiz.org>
parents: 1235
diff changeset
109 - 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
110 - Answer question by de commiter, this can also serve to train people