launchpad-dev team mailing list archive

[Martin Pool <mbp@xxxxxxxxxxxxx>]

On 21 October 2010 02:18, Deryck Hodge <deryck.hodge@xxxxxxxxxxxxx> wrote:
> Hi, Robert, Francis, and all.
>
> On Tue, Oct 19, 2010 at 2:50 PM, Francis J. Lacoste
> <francis.lacoste@xxxxxxxxxxxxx> wrote:
>> Hi Robert,
>>
>> I think this is a very worthwhile experiment.  I appreciate that you are
>> bringing a cost/benefit analysis to one of the sacred cow of Launchpad.
>>
>
> I didn't read Robert's initial proposal as a cost/benefit analysis.
> He and I talked a bit about this proposal yesterday and viewing in in
> that light, I'm more +1 than -1 now.  There are bits of the proposal
> and language used to describe the proposal that have me skeptical
> about it being a positive experiment, but I certainly understand what
> Robert is after now and support his efforts.
>
> To be clear about my skepticism, it's that I think most of our
> friction over reviews is that we don't have a shared understanding of
> the purpose of a review or even what code quality means (e.g. some
> think lint and formatting, others architectural issues, others module
> or function design, etc.), so I think all this experiment will show us
> is the number of people who opt out of review. :-)  Also, I don't have
> some great fear of plummeting code quality.  We're all responsible
> adults here. :-)  I guess it's more that I worry about inconsistent
> practices and statements, i.e. "we value peer-reviewed code, but you
> don't have to do it if you don't want to."

I said to somebody the other day that I care more about using reviews
to improve contributors than to improve code.  What I mean is that
it's less important to me that every contribution be as clean as
possible than that every contributor internalizes a shared model of
what is desirable in code (all the way from whitespace through to
architecture), and that they find participation to be fun.

I think people can have some fear that if reviews aren't strict, the
code will become incoherent or hard to read.   However you have to
remember that new contributors who aren't familiar with our practices
are very likely to be only writing small bits of code.  The bulk of
the code will be done by core contributors and so the question is to
what extent they agree on and care about certain qualities, and how
much they feel they have the time/environment that lets them make
things nice.

Code reviews can be a chance to show people how much you care about
helping them learn the code and get their change in.  I feel this is
the best way to get them to come back and improve more things - much
better than holding the original patch hostage until more work is
done.

At least as a thought experiment they could be optional for everyone.
If you have to force people to do them it suggests to me that they are
either taking too long or not focussing on things the contributor
actually finds useful.

iow: pace jml, "I love your patch, and you too."

-- 
Martin
who obviously should be doing bzr reviews not kibitzing ;-)