[asterisk-dev] Reviewboard Usage Guidelines

[Russell Bryant]

Tzafrir Cohen wrote:
> I'm not sure it scales. I already feel that this list is a bit swamped
> with reviewboard traffic and after a discussion begins I normally find
> it a bit difficult to step in the middle.

What doesn't scale about it exactly?  I feel that the traffic is
perfectly appropriate for the development list.

All code review messages have a [Code Review] prefix in the subject so
that they could be filtered separately for those that want to do so.

>> After seeing how many bugs have been found and fixed through in depth
>> code review on reviewboard before making it in to Asterisk, I am
>> inclined to say that I think _all_ non-trivial changes should be put
>> through reviewboard and require at least one sign off from another
>> committer before being merged.
> 
> Is this intended to replace the "test branch"?

I certainly do not imply that code review takes the place of testing.  I
am concerned with raising the bar for code quality before it makes it
into trunk (or a release branch).  Both code review and testing are
critical parts of that.

> I find the lack of integration between the reviewboard and SVN
> disappointing. I wish I could usbmit a test branch for review.

Have you seen the documentation I posted in doxygen?  post-review makes
it very easy to submit code from an svn checkout.  When the code is in a
branch, there is an extra step or two for now, but it's really not that
difficult.

The tools were written for pre-commit review, so it does not natively
have support for saying "please post a diff between these two branches",
but it wouldn't be too bad to add if someone wanted to do so.

-- 
Russell Bryant
Digium, Inc. | Senior Software Engineer, Open Source Team Lead
445 Jan Davis Drive NW - Huntsville, AL 35806 - USA
Check us out at: www.digium.com & www.asterisk.org