Indefero

Issue 39: Improving the code review

Reported by Loïc d'Anterroches, Aug 29, 2008

A nice feature would be code review. GoogleCode has introduced that, 
Rietveld is also available:
http://codereview.appspot.com/

I have tried the GoogleCode version and have a bad 
"feeling" about it. The interface is confusing with many 
ways to give feedback and the multiple steps to add comments (draft 
then publish). 

We can do better :)

Comment 1 by Xavier Brochard, Sep 11, 2008

Hello Loïc,
I was browsing here to add the same wish. I've just discovered 
Review Board http://www.review-board.org/  It's in PHP and use the 
Django framework.

Comment 2 by Loïc d'Anterroches, Sep 12, 2008

Hi Xavier, yes, this review board is quite nice, my question at the 
moment is more a problem of the way to handle the review requests.

A review is basically a request to check that a diff/patch is good 
enough to be included within the mainline and we need to have the 
tools to easily comment and improve until we reach something right.

My idea at the moment is to request the submitter of a review to 
upload a diff/patch against a given git commit or subversion 
revision.

Then it will be possible to attach to the diff/patch some general 
comments, like here about the issues or line by line comments (using 
a kind of AJAX interface). 

Still not fully sure about the approach, but I want it to be very 
simple for both the people submitting the review request and the 
reviewer.
Status: Accepted

Comment 3 by Loïc d'Anterroches, Nov 24, 2008

Ok, found a way to do it.

The principle will be to have a patch against a given revision of 
the code, the patch can be in the form of an already applied commit 
(we have the diff from the commit) view or from an uploaded patch.

Post commit: you select a commit/revision in the source view and you 
click on a "Start review" button. We have all the base 
data for the review.

Pre commit: you go in the code review tab, give the reference 
commit/revision, upload a patch.

For both pre/post commit, you give a little description and provide 
a list of people you want to review your code.

You submit the form. A review request is created and emails to the 
invited reviewers are sent.

On the review page, you can see on the left the set of original 
files and on the right the new version, everything with red/green 
colours to highlight the diffs.

The reviewers and the submitters can easily had comments, either at 
a line level or at the complete file level. The comments are 
"pending" until the end of the review. When the reviewer 
has finished, he can "publish his review" and the comments 
are made available to others, an email is also sent to the submitter 
of the review.

From a technical point of view, the only difficult part is to apply 
the patch in the case of a pre commit review. We want to be able to 
apply a path without the need to run the patch program.

From a GUI point of view, it will requires javascript for the inline 
comments. Full file comments will not require javascript. I will 
make it so Javascript is *not* mandatory, that way people without 
javascript will still be able to put full file comments.
Labels: Priority:Medium Priority:Low

Comment 4 by Loïc d'Anterroches, Nov 30, 2008

Started in commit f690968b1149970a989bfa. Now that the basis is 
there, it requires polishing.
Status: Started
Owner: loic

Comment 5 by Loïc d'Anterroches, Nov 30, 2008

What needs to be done for the code review:

- Notifications for the reviews.
- Select a commit to create a code review.
- Update a code review with a new patch.
- Change the status of a code review.
Labels: Type:Defect Type:Enhancement
Summary: Improving the code review

Comment 6 by Loïc d'Anterroches, Dec 2, 2008

Labels: Type:Enhancement Milestone:Release1.0 Type:Defect

Comment 7 by Andrey Kulikov, May 28, 2009

It'll be VERY useful to make possible to attach comments to each 
separate line, not only just to the whole changed file.
This feature exist in CodeCollaborator (comercial tool).
I've use it, and found it very convenient.

Comment 8 by Loïc d'Anterroches, Jun 29, 2009

<chuck> da-loic: I created some simple-ish CSS/JS magic for 
doing per-line comments in code review if you want
<da-loic> chuck: yes yes yes
<chuck> da-loic: kk, it's not completely finished, but here's 
a demo (http://melbye.nfshost.com/codereview/) and here's the 
archive of it (http://melbye.nfshost.com/codereview.tar.gz)
<da-loic> chuck: great, adding that to the ticket

Comment 9 by Charles Melbye, Jun 29, 2009

The archive of a library to manage comments on a per line basis is 
attached. It's in its preliminary stages, and it might have to be 
refactored a bit. For example, it doesn't actually know how to 
display the actual comments yet, but that should be the easy part.

Comment 10 by Loïc d'Anterroches, Jul 15, 2009

Working an it now, my steps are:

1. get the same functionalities as now but fixing issue 145 and 
adding the ability to propose new patches for a given review.

2. get the comments at the line level, the backend will have the 
support of the lines already at step 1, this means it will be 
possible to use the reviews safely when step 1 will be done.

Comment 11 by Loïc d'Anterroches, Jul 15, 2009

I have the new DB schema definition, I now need to update the UI to 
match it. I will make a code sprint to get that done tonight. 

Considered as a high priority task.
Labels: Priority:High Priority:Medium

Comment 12 by Julien Huang, Oct 30, 2009

My 2 cents, there may be some ideas/concepts to get from Gerrit.

Rietveld from Google has been forked (by Google also) into 
"Gerrit" (http://gerrit.googlecode.com/).

Gerrit is being used for the Android OS project :
https://review.source.android.com/

The submission workflow ("Gerrit inside") :
http://source.android.com/submit-patches
http://source.android.com/submit-patches/workflow

Comment 13 by Loïc d'Anterroches, Oct 30, 2009

Great, especially the last two links. Thanks!

Comment 14 by Jakub Vitak, Nov 2, 2009

please see Issue 326

Comment 15 by Loïc d'Anterroches, Feb 14, 2010

Labels: Milestone:Release1.0

Created: 4 years 8 months ago by Loïc d'Anterroches

Updated: 3 years 3 months ago

Status: Started

Owner: Loïc d'Anterroches

Followed by: 7 persons

Labels:
Priority:High
Type:Enhancement