Mercurial > pylearn

annotate doc/v2_planning/code_review.txt @ 1377:0665274b14af

Find changesets by keywords (author, files, the commit message), revision number or hash, or revset expression.
Minor fixes for better Sphinx doc output
author Olivier Delalleau <delallea@iro>
date Thu, 18 Nov 2010 14:14:48 -0500
parents 17e9206af22b
children b68f4d7e4df3
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
1314
17e9206af22b updated the coding_review file and added our proposed politic
Frederic Bastien <nouiz@nouiz.org>
parents: 1263
diff changeset
13 - Install Review Board and try it
17e9206af22b updated the coding_review file and added our proposed politic
Frederic Bastien <nouiz@nouiz.org>
parents: 1263
diff changeset
14 - test review of merge
17e9206af22b updated the coding_review file and added our proposed politic
Frederic Bastien <nouiz@nouiz.org>
parents: 1263
diff changeset
15 - how work with branch
17e9206af22b updated the coding_review file and added our proposed politic
Frederic Bastien <nouiz@nouiz.org>
parents: 1263
diff changeset
16 - Write our proposed politic
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
1314
17e9206af22b updated the coding_review file and added our proposed politic
Frederic Bastien <nouiz@nouiz.org>
parents: 1263
diff changeset
33 unreviewed commits, more esthetics, handle many repo, keep assemble easily)
1263
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/
1314
17e9206af22b updated the coding_review file and added our proposed politic
Frederic Bastien <nouiz@nouiz.org>
parents: 1263
diff changeset
57 - post-commit
17e9206af22b updated the coding_review file and added our proposed politic
Frederic Bastien <nouiz@nouiz.org>
parents: 1263
diff changeset
58 - no list of not reviewed commit
17e9206af22b updated the coding_review file and added our proposed politic
Frederic Bastien <nouiz@nouiz.org>
parents: 1263
diff changeset
59 - no python syntax highlight
17e9206af22b updated the coding_review file and added our proposed politic
Frederic Bastien <nouiz@nouiz.org>
parents: 1263
diff changeset
60 - weird comment by line
17e9206af22b updated the coding_review file and added our proposed politic
Frederic Bastien <nouiz@nouiz.org>
parents: 1263
diff changeset
61 - diff of merge seam bugged
1263
10113a1050ce More RST
Pascal Lamblin <lamblinp@iro.umontreal.ca>
parents: 1246
diff changeset
62 - Maybe
1209
5ff1d375fc33 added a first list of code review system.
Frederic Bastien <nouiz@nouiz.org>
parents:
diff changeset
63
1235
a9b601119197 added project to review and other stuff in code_review.txt
Frederic Bastien <nouiz@nouiz.org>
parents: 1233
diff changeset
64 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
65 ---------------------------------------
1209
5ff1d375fc33 added a first list of code review system.
Frederic Bastien <nouiz@nouiz.org>
parents:
diff changeset
66
5ff1d375fc33 added a first list of code review system.
Frederic Bastien <nouiz@nouiz.org>
parents:
diff changeset
67 - integrate with our ticket system?
5ff1d375fc33 added a first list of code review system.
Frederic Bastien <nouiz@nouiz.org>
parents:
diff changeset
68 - 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
69 - 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
70 - 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
71 - 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
72 - 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
73 - 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
74 - syntax highlight
1246
14444845989a add stuff to code review following the first meeting.
Frederic Bastien <nouiz@nouiz.org>
parents: 1235
diff changeset
75 - 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
76 - 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
77 - 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
78
a9b601119197 added project to review and other stuff in code_review.txt
Frederic Bastien <nouiz@nouiz.org>
parents: 1233
diff changeset
79 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
80 ------------------
a9b601119197 added project to review and other stuff in code_review.txt
Frederic Bastien <nouiz@nouiz.org>
parents: 1233
diff changeset
81
a9b601119197 added project to review and other stuff in code_review.txt
Frederic Bastien <nouiz@nouiz.org>
parents: 1233
diff changeset
82 - 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
83 - 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
84 - 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
85
1246
14444845989a add stuff to code review following the first meeting.
Frederic Bastien <nouiz@nouiz.org>
parents: 1235
diff changeset
86 Type of code review
14444845989a add stuff to code review following the first meeting.
Frederic Bastien <nouiz@nouiz.org>
parents: 1235
diff changeset
87 -------------------
1235
a9b601119197 added project to review and other stuff in code_review.txt
Frederic Bastien <nouiz@nouiz.org>
parents: 1233
diff changeset
88
1246
14444845989a add stuff to code review following the first meeting.
Frederic Bastien <nouiz@nouiz.org>
parents: 1235
diff changeset
89 - 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
90 - 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
91 - 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
92 - 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
93 - 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
94 - Alternative: Test-Driven development
14444845989a add stuff to code review following the first meeting.
Frederic Bastien <nouiz@nouiz.org>
parents: 1235
diff changeset
95 - 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
96
14444845989a add stuff to code review following the first meeting.
Frederic Bastien <nouiz@nouiz.org>
parents: 1235
diff changeset
97 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
98
14444845989a add stuff to code review following the first meeting.
Frederic Bastien <nouiz@nouiz.org>
parents: 1235
diff changeset
99 Reason for the code 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 - 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
103 - This help to find better solution to problem
1314
17e9206af22b updated the coding_review file and added our proposed politic
Frederic Bastien <nouiz@nouiz.org>
parents: 1263
diff changeset
104 - This help to train people on our tools and framework.
1246
14444845989a add stuff to code review following the first meeting.
Frederic Bastien <nouiz@nouiz.org>
parents: 1235
diff changeset
105
14444845989a add stuff to code review following the first meeting.
Frederic Bastien <nouiz@nouiz.org>
parents: 1235
diff changeset
106 Check list for review
14444845989a add stuff to code review following the first meeting.
Frederic Bastien <nouiz@nouiz.org>
parents: 1235
diff changeset
107 ---------------------
14444845989a add stuff to code review following the first meeting.
Frederic Bastien <nouiz@nouiz.org>
parents: 1235
diff changeset
108
14444845989a add stuff to code review following the first meeting.
Frederic Bastien <nouiz@nouiz.org>
parents: 1235
diff changeset
109 - 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
110 - 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
111 - 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
112 - 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
113 - 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
114 - 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
115 - Is the code clear/comprehensible
14444845989a add stuff to code review following the first meeting.
Frederic Bastien <nouiz@nouiz.org>
parents: 1235
diff changeset
116 - 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
117 - Answer question by de commiter, this can also serve to train people
1314
17e9206af22b updated the coding_review file and added our proposed politic
Frederic Bastien <nouiz@nouiz.org>
parents: 1263
diff changeset
118 - Check for typo
17e9206af22b updated the coding_review file and added our proposed politic
Frederic Bastien <nouiz@nouiz.org>
parents: 1263
diff changeset
119 - No debug code(print, breakpoint,...)
17e9206af22b updated the coding_review file and added our proposed politic
Frederic Bastien <nouiz@nouiz.org>
parents: 1263
diff changeset
120 - If commit message tell to don't review, check that the code is disabled by default and that when enabled print a warning.
17e9206af22b updated the coding_review file and added our proposed politic
Frederic Bastien <nouiz@nouiz.org>
parents: 1263
diff changeset
121
17e9206af22b updated the coding_review file and added our proposed politic
Frederic Bastien <nouiz@nouiz.org>
parents: 1263
diff changeset
122 Proposed Politic
17e9206af22b updated the coding_review file and added our proposed politic
Frederic Bastien <nouiz@nouiz.org>
parents: 1263
diff changeset
123 ----------------
17e9206af22b updated the coding_review file and added our proposed politic
Frederic Bastien <nouiz@nouiz.org>
parents: 1263
diff changeset
124
17e9206af22b updated the coding_review file and added our proposed politic
Frederic Bastien <nouiz@nouiz.org>
parents: 1263
diff changeset
125 - For each commit message, if the author don't want this commit to be reviewed, tell it in the message
17e9206af22b updated the coding_review file and added our proposed politic
Frederic Bastien <nouiz@nouiz.org>
parents: 1263
diff changeset
126 - Usefull for experimental repository, not Theano
17e9206af22b updated the coding_review file and added our proposed politic
Frederic Bastien <nouiz@nouiz.org>
parents: 1263
diff changeset
127 - 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.
17e9206af22b updated the coding_review file and added our proposed politic
Frederic Bastien <nouiz@nouiz.org>
parents: 1263
diff changeset
128 - Reviewer should check that the check is not enabled by default and when enabled should print a warning.
17e9206af22b updated the coding_review file and added our proposed politic
Frederic Bastien <nouiz@nouiz.org>
parents: 1263
diff changeset
129 - We check all commit to Theano, Pylearn and Jobman.(Official tools)
17e9206af22b updated the coding_review file and added our proposed politic
Frederic Bastien <nouiz@nouiz.org>
parents: 1263
diff changeset
130 - We check experimental repos when asked.
17e9206af22b updated the coding_review file and added our proposed politic
Frederic Bastien <nouiz@nouiz.org>
parents: 1263
diff changeset
131 - One official reviewer per week.
17e9206af22b updated the coding_review file and added our proposed politic
Frederic Bastien <nouiz@nouiz.org>
parents: 1263
diff changeset
132 - He review all code and ask expert when needed.
17e9206af22b updated the coding_review file and added our proposed politic
Frederic Bastien <nouiz@nouiz.org>
parents: 1263
diff changeset
133 - Should check the check list again all review.
17e9206af22b updated the coding_review file and added our proposed politic
Frederic Bastien <nouiz@nouiz.org>
parents: 1263
diff changeset
134 - We choose the reviewer in the theano user of the lab with commit right.
17e9206af22b updated the coding_review file and added our proposed politic
Frederic Bastien <nouiz@nouiz.org>
parents: 1263
diff changeset
135 - On fait une list d'expert par demain de problem(gpu, optimization, algo,...)
17e9206af22b updated the coding_review file and added our proposed politic
Frederic Bastien <nouiz@nouiz.org>
parents: 1263
diff changeset
136 - If some body break the build bot, it is him the reviewer for the next week
17e9206af22b updated the coding_review file and added our proposed politic
Frederic Bastien <nouiz@nouiz.org>
parents: 1263
diff changeset
137 - Maximum of one week by mount.
17e9206af22b updated the coding_review file and added our proposed politic
Frederic Bastien <nouiz@nouiz.org>
parents: 1263
diff changeset
138 - If their is big week or during rush that include every body of the lab, we can change more frequently.
17e9206af22b updated the coding_review file and added our proposed politic
Frederic Bastien <nouiz@nouiz.org>
parents: 1263
diff changeset
139 - If a commit have problem, it is the original reviewer that should make the follow up.