Mercurial > pylearn

annotate doc/v2_planning/code_review.txt @ 1398:1934ba31b7d9

Find changesets by keywords (author, files, the commit message), revision number or hash, or revset expression.
rst syntax fix and update to what is currently done.
author Frederic Bastien <nouiz@nouiz.org>
date Wed, 12 Jan 2011 17:03:30 -0500
parents 0ff6c613cdf0
children
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
1388
0ff6c613cdf0 Many small fixes in code review proposal
Olivier Delalleau <delallea@iro>
parents: 1385
diff changeset
13 - Test the 2 proposed workflows (review board and github)
0ff6c613cdf0 Many small fixes in code review proposal
Olivier Delalleau <delallea@iro>
parents: 1385
diff changeset
14
0ff6c613cdf0 Many small fixes in code review proposal
Olivier Delalleau <delallea@iro>
parents: 1385
diff changeset
15 Reasons for code review
0ff6c613cdf0 Many small fixes in code review proposal
Olivier Delalleau <delallea@iro>
parents: 1385
diff changeset
16 -----------------------
1385
8b857d141761 update the proposed politics.
Frederic Bastien <nouiz@nouiz.org>
parents: 1384
diff changeset
17
1388
0ff6c613cdf0 Many small fixes in code review proposal
Olivier Delalleau <delallea@iro>
parents: 1385
diff changeset
18 - We want at least 2 people to read all code. That means we need a reviewer.
0ff6c613cdf0 Many small fixes in code review proposal
Olivier Delalleau <delallea@iro>
parents: 1385
diff changeset
19 - This helps to find better solutions to problems.
0ff6c613cdf0 Many small fixes in code review proposal
Olivier Delalleau <delallea@iro>
parents: 1385
diff changeset
20 - This helps to train people on our tools and framework.
0ff6c613cdf0 Many small fixes in code review proposal
Olivier Delalleau <delallea@iro>
parents: 1385
diff changeset
21 - This gives better code and less bugs in the end (everybody makes mistakes).
1385
8b857d141761 update the proposed politics.
Frederic Bastien <nouiz@nouiz.org>
parents: 1384
diff changeset
22
1388
0ff6c613cdf0 Many small fixes in code review proposal
Olivier Delalleau <delallea@iro>
parents: 1385
diff changeset
23 Proposed Policy
0ff6c613cdf0 Many small fixes in code review proposal
Olivier Delalleau <delallea@iro>
parents: 1385
diff changeset
24 ---------------
0ff6c613cdf0 Many small fixes in code review proposal
Olivier Delalleau <delallea@iro>
parents: 1385
diff changeset
25
0ff6c613cdf0 Many small fixes in code review proposal
Olivier Delalleau <delallea@iro>
parents: 1385
diff changeset
26 There are 2 proposals we want to test:
1385
8b857d141761 update the proposed politics.
Frederic Bastien <nouiz@nouiz.org>
parents: 1384
diff changeset
27
1388
0ff6c613cdf0 Many small fixes in code review proposal
Olivier Delalleau <delallea@iro>
parents: 1385
diff changeset
28 - GitHub fork/merge/pull request for new Pylearn
0ff6c613cdf0 Many small fixes in code review proposal
Olivier Delalleau <delallea@iro>
parents: 1385
diff changeset
29 - When someone asks for a pull request, someone else does the review.
0ff6c613cdf0 Many small fixes in code review proposal
Olivier Delalleau <delallea@iro>
parents: 1385
diff changeset
30 - Everyone should work within their own Pylearn fork, and submit pull
0ff6c613cdf0 Many small fixes in code review proposal
Olivier Delalleau <delallea@iro>
parents: 1385
diff changeset
31 requests to merge their code into the master Pylearn repository.
1385
8b857d141761 update the proposed politics.
Frederic Bastien <nouiz@nouiz.org>
parents: 1384
diff changeset
32 - Review Board post-commit for Theano
1388
0ff6c613cdf0 Many small fixes in code review proposal
Olivier Delalleau <delallea@iro>
parents: 1385
diff changeset
33 - For each commit message, if the author does not want this commit to be reviewed, say it in the message
0ff6c613cdf0 Many small fixes in code review proposal
Olivier Delalleau <delallea@iro>
parents: 1385
diff changeset
34 - 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.
0ff6c613cdf0 Many small fixes in code review proposal
Olivier Delalleau <delallea@iro>
parents: 1385
diff changeset
35 - Reviewer should still check that it is not enabled by default and when enabled it prints a warning (once per job execution).
0ff6c613cdf0 Many small fixes in code review proposal
Olivier Delalleau <delallea@iro>
parents: 1385
diff changeset
36 - We check all commits to Theano and Jobman (official tools).
0ff6c613cdf0 Many small fixes in code review proposal
Olivier Delalleau <delallea@iro>
parents: 1385
diff changeset
37 - If the official reviewer is not the right person for the task (gpu code, ...)
0ff6c613cdf0 Many small fixes in code review proposal
Olivier Delalleau <delallea@iro>
parents: 1385
diff changeset
38 he has the responsability to find someone else (ask people in the lab, mailing-list, ...).
0ff6c613cdf0 Many small fixes in code review proposal
Olivier Delalleau <delallea@iro>
parents: 1385
diff changeset
39 - The official reviewer should:
0ff6c613cdf0 Many small fixes in code review proposal
Olivier Delalleau <delallea@iro>
parents: 1385
diff changeset
40 - Review all code (see the checklist) and ask an expert when needed.
0ff6c613cdf0 Many small fixes in code review proposal
Olivier Delalleau <delallea@iro>
parents: 1385
diff changeset
41 - Commit easy fixes and ask the original committer to review them
0ff6c613cdf0 Many small fixes in code review proposal
Olivier Delalleau <delallea@iro>
parents: 1385
diff changeset
42 (non-trivial fixes are the responsibility of the original committer).
0ff6c613cdf0 Many small fixes in code review proposal
Olivier Delalleau <delallea@iro>
parents: 1385
diff changeset
43 - The official reviewer is chosen among the Theano users in the lab with commit rights.
0ff6c613cdf0 Many small fixes in code review proposal
Olivier Delalleau <delallea@iro>
parents: 1385
diff changeset
44 - In this test, we ask the official reviewer to review all commits from one day.
0ff6c613cdf0 Many small fixes in code review proposal
Olivier Delalleau <delallea@iro>
parents: 1385
diff changeset
45 - We will set up a list of experts by domain (gpu, optimization, algo,...).
0ff6c613cdf0 Many small fixes in code review proposal
Olivier Delalleau <delallea@iro>
parents: 1385
diff changeset
46 - After this test, we may want to make it longer (1 week?)
0ff6c613cdf0 Many small fixes in code review proposal
Olivier Delalleau <delallea@iro>
parents: 1385
diff changeset
47 - If somebody breaks the build bot, he becomes the reviewer for the next week/days.
1385
8b857d141761 update the proposed politics.
Frederic Bastien <nouiz@nouiz.org>
parents: 1384
diff changeset
48 - Maximum of one week by month.
1388
0ff6c613cdf0 Many small fixes in code review proposal
Olivier Delalleau <delallea@iro>
parents: 1385
diff changeset
49 - If there is a big week or a rush that includes everybody in the lab, we can change more frequently.
0ff6c613cdf0 Many small fixes in code review proposal
Olivier Delalleau <delallea@iro>
parents: 1385
diff changeset
50 - If a commit has problems, the original reviewer should follow-up on it.
1385
8b857d141761 update the proposed politics.
Frederic Bastien <nouiz@nouiz.org>
parents: 1384
diff changeset
51
1388
0ff6c613cdf0 Many small fixes in code review proposal
Olivier Delalleau <delallea@iro>
parents: 1385
diff changeset
52 Other general comments:
0ff6c613cdf0 Many small fixes in code review proposal
Olivier Delalleau <delallea@iro>
parents: 1385
diff changeset
53 - We should never be the official reviewer of our own code. When this happens,
0ff6c613cdf0 Many small fixes in code review proposal
Olivier Delalleau <delallea@iro>
parents: 1385
diff changeset
54 ask the reviewer for the next day to take care of it.
0ff6c613cdf0 Many small fixes in code review proposal
Olivier Delalleau <delallea@iro>
parents: 1385
diff changeset
55 - Experimental code (not in Theano) may be tagged as not being reviewed (in
0ff6c613cdf0 Many small fixes in code review proposal
Olivier Delalleau <delallea@iro>
parents: 1385
diff changeset
56 commit message).
1385
8b857d141761 update the proposed politics.
Frederic Bastien <nouiz@nouiz.org>
parents: 1384
diff changeset
57
1388
0ff6c613cdf0 Many small fixes in code review proposal
Olivier Delalleau <delallea@iro>
parents: 1385
diff changeset
58 Checklist for review
0ff6c613cdf0 Many small fixes in code review proposal
Olivier Delalleau <delallea@iro>
parents: 1385
diff changeset
59 --------------------
1385
8b857d141761 update the proposed politics.
Frederic Bastien <nouiz@nouiz.org>
parents: 1384
diff changeset
60
1388
0ff6c613cdf0 Many small fixes in code review proposal
Olivier Delalleau <delallea@iro>
parents: 1385
diff changeset
61 - Are there tests and do they cover all cases?
0ff6c613cdf0 Many small fixes in code review proposal
Olivier Delalleau <delallea@iro>
parents: 1385
diff changeset
62 - Is there documentation in the code file?
0ff6c613cdf0 Many small fixes in code review proposal
Olivier Delalleau <delallea@iro>
parents: 1385
diff changeset
63 - Should public (HTML) documentation be updated as well?
0ff6c613cdf0 Many small fixes in code review proposal
Olivier Delalleau <delallea@iro>
parents: 1385
diff changeset
64 - Are additions well integrated into our framework?
0ff6c613cdf0 Many small fixes in code review proposal
Olivier Delalleau <delallea@iro>
parents: 1385
diff changeset
65 - Is the code placed in the right files, and the right place in those files?
0ff6c613cdf0 Many small fixes in code review proposal
Olivier Delalleau <delallea@iro>
parents: 1385
diff changeset
66 - Try not to duplicate code.
0ff6c613cdf0 Many small fixes in code review proposal
Olivier Delalleau <delallea@iro>
parents: 1385
diff changeset
67 - Is the code clear and easy to understand?
0ff6c613cdf0 Many small fixes in code review proposal
Olivier Delalleau <delallea@iro>
parents: 1385
diff changeset
68 - Are there comments describing what is being done?
0ff6c613cdf0 Many small fixes in code review proposal
Olivier Delalleau <delallea@iro>
parents: 1385
diff changeset
69 - Answer potential questions by the committer (in the code). This can also help to train people.
0ff6c613cdf0 Many small fixes in code review proposal
Olivier Delalleau <delallea@iro>
parents: 1385
diff changeset
70 - Check for typos.
0ff6c613cdf0 Many small fixes in code review proposal
Olivier Delalleau <delallea@iro>
parents: 1385
diff changeset
71 - No debug code (print, breakpoints, ...).
0ff6c613cdf0 Many small fixes in code review proposal
Olivier Delalleau <delallea@iro>
parents: 1385
diff changeset
72 - If commit message says not to review, check that the code is disabled by default and that when enabled prints a warning.
0ff6c613cdf0 Many small fixes in code review proposal
Olivier Delalleau <delallea@iro>
parents: 1385
diff changeset
73 - Check for conformity to our coding guidelines.
1385
8b857d141761 update the proposed politics.
Frederic Bastien <nouiz@nouiz.org>
parents: 1384
diff changeset
74
8b857d141761 update the proposed politics.
Frederic Bastien <nouiz@nouiz.org>
parents: 1384
diff changeset
75 Some system that we checked
8b857d141761 update the proposed politics.
Frederic Bastien <nouiz@nouiz.org>
parents: 1384
diff changeset
76 ---------------------------
1209
5ff1d375fc33 added a first list of code review system.
Frederic Bastien <nouiz@nouiz.org>
parents:
diff changeset
77
1384
b68f4d7e4df3 small old modif not commtited.
Frederic Bastien <nouiz@nouiz.org>
parents: 1314
diff changeset
78 - `Review Board <http://www.reviewboard.org>`_
b68f4d7e4df3 small old modif not commtited.
Frederic Bastien <nouiz@nouiz.org>
parents: 1314
diff changeset
79 - Interesting, but some questions remain (how well it integrates with hg,
b68f4d7e4df3 small old modif not commtited.
Frederic Bastien <nouiz@nouiz.org>
parents: 1314
diff changeset
80 notably)
b68f4d7e4df3 small old modif not commtited.
Frederic Bastien <nouiz@nouiz.org>
parents: 1314
diff changeset
81 - Some advantages over Google code (comment on multi-line chunks, list of
b68f4d7e4df3 small old modif not commtited.
Frederic Bastien <nouiz@nouiz.org>
parents: 1314
diff changeset
82 unreviewed commits, more esthetics, handle many repo, keep assemble easily)
b68f4d7e4df3 small old modif not commtited.
Frederic Bastien <nouiz@nouiz.org>
parents: 1314
diff changeset
83 - Fred will install it so we can test it more thoroughly
1385
8b857d141761 update the proposed politics.
Frederic Bastien <nouiz@nouiz.org>
parents: 1384
diff changeset
84 - `GitHub pull request <https://github.com/blog/712-pull-requests-2-0>`_
8b857d141761 update the proposed politics.
Frederic Bastien <nouiz@nouiz.org>
parents: 1384
diff changeset
85 - pre-commit review
1384
b68f4d7e4df3 small old modif not commtited.
Frederic Bastien <nouiz@nouiz.org>
parents: 1314
diff changeset
86
b68f4d7e4df3 small old modif not commtited.
Frederic Bastien <nouiz@nouiz.org>
parents: 1314
diff changeset
87 - `Google Code <http://code.google.com/p/support/wiki/CodeReviews>`_
b68f4d7e4df3 small old modif not commtited.
Frederic Bastien <nouiz@nouiz.org>
parents: 1314
diff changeset
88 - Test bench with a clone of Theano at
b68f4d7e4df3 small old modif not commtited.
Frederic Bastien <nouiz@nouiz.org>
parents: 1314
diff changeset
89 http://code.google.com/p/theanoclone/
b68f4d7e4df3 small old modif not commtited.
Frederic Bastien <nouiz@nouiz.org>
parents: 1314
diff changeset
90 - post-commit
b68f4d7e4df3 small old modif not commtited.
Frederic Bastien <nouiz@nouiz.org>
parents: 1314
diff changeset
91 - no list of not reviewed commit
b68f4d7e4df3 small old modif not commtited.
Frederic Bastien <nouiz@nouiz.org>
parents: 1314
diff changeset
92 - no python syntax highlight
b68f4d7e4df3 small old modif not commtited.
Frederic Bastien <nouiz@nouiz.org>
parents: 1314
diff changeset
93 - weird comment by line
b68f4d7e4df3 small old modif not commtited.
Frederic Bastien <nouiz@nouiz.org>
parents: 1314
diff changeset
94 - diff of merge seam bugged
b68f4d7e4df3 small old modif not commtited.
Frederic Bastien <nouiz@nouiz.org>
parents: 1314
diff changeset
95 - Maybe
b68f4d7e4df3 small old modif not commtited.
Frederic Bastien <nouiz@nouiz.org>
parents: 1314
diff changeset
96
b68f4d7e4df3 small old modif not commtited.
Frederic Bastien <nouiz@nouiz.org>
parents: 1314
diff changeset
97 - `Kiln <http://fogcreek.com/Kiln/LearnMore.html?section=StartReviewsEffortlessly>`_
b68f4d7e4df3 small old modif not commtited.
Frederic Bastien <nouiz@nouiz.org>
parents: 1314
diff changeset
98
1263
10113a1050ce More RST
Pascal Lamblin <lamblinp@iro.umontreal.ca>
parents: 1246
diff changeset
99 - `rietveld <http://code.google.com/p/rietveld/>`_
10113a1050ce More RST
Pascal Lamblin <lamblinp@iro.umontreal.ca>
parents: 1246
diff changeset
100 - Made by Guido van Rossum, seam basic and svn only
10113a1050ce More RST
Pascal Lamblin <lamblinp@iro.umontreal.ca>
parents: 1246
diff changeset
101 - No, not enough features
10113a1050ce More RST
Pascal Lamblin <lamblinp@iro.umontreal.ca>
parents: 1246
diff changeset
102
10113a1050ce More RST
Pascal Lamblin <lamblinp@iro.umontreal.ca>
parents: 1246
diff changeset
103 - `Gerrit <http://code.google.com/p/gerrit/>`_
10113a1050ce More RST
Pascal Lamblin <lamblinp@iro.umontreal.ca>
parents: 1246
diff changeset
104 - git only
10113a1050ce More RST
Pascal Lamblin <lamblinp@iro.umontreal.ca>
parents: 1246
diff changeset
105 - No
10113a1050ce More RST
Pascal Lamblin <lamblinp@iro.umontreal.ca>
parents: 1246
diff changeset
106
10113a1050ce More RST
Pascal Lamblin <lamblinp@iro.umontreal.ca>
parents: 1246
diff changeset
107 - `Code Striker <http://codestriker.sourceforge.net/>`_
10113a1050ce More RST
Pascal Lamblin <lamblinp@iro.umontreal.ca>
parents: 1246
diff changeset
108 - 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
109 - Seems less interesting than Review Board
10113a1050ce More RST
Pascal Lamblin <lamblinp@iro.umontreal.ca>
parents: 1246
diff changeset
110 - No
10113a1050ce More RST
Pascal Lamblin <lamblinp@iro.umontreal.ca>
parents: 1246
diff changeset
111
10113a1050ce More RST
Pascal Lamblin <lamblinp@iro.umontreal.ca>
parents: 1246
diff changeset
112 - `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
113 - No
10113a1050ce More RST
Pascal Lamblin <lamblinp@iro.umontreal.ca>
parents: 1246
diff changeset
114
10113a1050ce More RST
Pascal Lamblin <lamblinp@iro.umontreal.ca>
parents: 1246
diff changeset
115 - `track PeerReviewPlugin <http://trac-hacks.org/wiki/PeerReviewPlugin>`_
10113a1050ce More RST
Pascal Lamblin <lamblinp@iro.umontreal.ca>
parents: 1246
diff changeset
116 - 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
117 - No
10113a1050ce More RST
Pascal Lamblin <lamblinp@iro.umontreal.ca>
parents: 1246
diff changeset
118
1384
b68f4d7e4df3 small old modif not commtited.
Frederic Bastien <nouiz@nouiz.org>
parents: 1314
diff changeset
119 - `track CodeReviewPlugin <http://trac-hacks.org/wiki/CodeReviewPlugin/Concepts>`_
b68f4d7e4df3 small old modif not commtited.
Frederic Bastien <nouiz@nouiz.org>
parents: 1314
diff changeset
120
b68f4d7e4df3 small old modif not commtited.
Frederic Bastien <nouiz@nouiz.org>
parents: 1314
diff changeset
121 - `track ExoWebCodeReviewPlugin <http://trac-hacks.org/wiki/ExoWebCodeReviewPlugin>`_
b68f4d7e4df3 small old modif not commtited.
Frederic Bastien <nouiz@nouiz.org>
parents: 1314
diff changeset
122
1263
10113a1050ce More RST
Pascal Lamblin <lamblinp@iro.umontreal.ca>
parents: 1246
diff changeset
123 - `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
124 - No (we will not wait until the feature is added...)
10113a1050ce More RST
Pascal Lamblin <lamblinp@iro.umontreal.ca>
parents: 1246
diff changeset
125
10113a1050ce More RST
Pascal Lamblin <lamblinp@iro.umontreal.ca>
parents: 1246
diff changeset
126 - `JCR <http://jcodereview.sourceforge.net/>`_
10113a1050ce More RST
Pascal Lamblin <lamblinp@iro.umontreal.ca>
parents: 1246
diff changeset
127 - No
10113a1050ce More RST
Pascal Lamblin <lamblinp@iro.umontreal.ca>
parents: 1246
diff changeset
128
1209
5ff1d375fc33 added a first list of code review system.
Frederic Bastien <nouiz@nouiz.org>
parents:
diff changeset
129
1235
a9b601119197 added project to review and other stuff in code_review.txt
Frederic Bastien <nouiz@nouiz.org>
parents: 1233
diff changeset
130 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
131 ---------------------------------------
1209
5ff1d375fc33 added a first list of code review system.
Frederic Bastien <nouiz@nouiz.org>
parents:
diff changeset
132
5ff1d375fc33 added a first list of code review system.
Frederic Bastien <nouiz@nouiz.org>
parents:
diff changeset
133 - integrate with our ticket system?
5ff1d375fc33 added a first list of code review system.
Frederic Bastien <nouiz@nouiz.org>
parents:
diff changeset
134 - 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
135 - work with mercurial, git?
1314
17e9206af22b updated the coding_review file and added our proposed politic
Frederic Bastien <nouiz@nouiz.org>
parents: 1263
diff changeset
136 - how branch/merge are handled
1246
14444845989a add stuff to code review following the first meeting.
Frederic Bastien <nouiz@nouiz.org>
parents: 1235
diff changeset
137 - 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
138 - check experimental repository code when asked
1314
17e9206af22b updated the coding_review file and added our proposed politic
Frederic Bastien <nouiz@nouiz.org>
parents: 1263
diff changeset
139 - how show diff? diff as vimdiff? patch?
17e9206af22b updated the coding_review file and added our proposed politic
Frederic Bastien <nouiz@nouiz.org>
parents: 1263
diff changeset
140 - syntax highlight
1246
14444845989a add stuff to code review following the first meeting.
Frederic Bastien <nouiz@nouiz.org>
parents: 1235
diff changeset
141 - 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
142 - 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
143 - 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
144
a9b601119197 added project to review and other stuff in code_review.txt
Frederic Bastien <nouiz@nouiz.org>
parents: 1233
diff changeset
145 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
146 ------------------
a9b601119197 added project to review and other stuff in code_review.txt
Frederic Bastien <nouiz@nouiz.org>
parents: 1233
diff changeset
147
a9b601119197 added project to review and other stuff in code_review.txt
Frederic Bastien <nouiz@nouiz.org>
parents: 1233
diff changeset
148 - 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
149 - 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
150 - 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
151
1246
14444845989a add stuff to code review following the first meeting.
Frederic Bastien <nouiz@nouiz.org>
parents: 1235
diff changeset
152 Type of code review
14444845989a add stuff to code review following the first meeting.
Frederic Bastien <nouiz@nouiz.org>
parents: 1235
diff changeset
153 -------------------
1235
a9b601119197 added project to review and other stuff in code_review.txt
Frederic Bastien <nouiz@nouiz.org>
parents: 1233
diff changeset
154
1246
14444845989a add stuff to code review following the first meeting.
Frederic Bastien <nouiz@nouiz.org>
parents: 1235
diff changeset
155 - 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
156 - 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
157 - 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
158 - 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
159 - 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
160 - Alternative: Test-Driven development
14444845989a add stuff to code review following the first meeting.
Frederic Bastien <nouiz@nouiz.org>
parents: 1235
diff changeset
161 - 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
162
14444845989a add stuff to code review following the first meeting.
Frederic Bastien <nouiz@nouiz.org>
parents: 1235
diff changeset
163 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
164