lilypond-devel
[Top][All Lists]
Advanced

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: review process not working


From: David Kastrup
Subject: Re: review process not working
Date: Tue, 26 Jul 2011 19:36:43 +0200
User-agent: Gnus/5.13 (Gnus v5.13) Emacs/24.0.50 (gnu/linux)

Graham Percival <address@hidden> writes:

> On Tue, Jul 26, 2011 at 12:05:31PM +0200, David Kastrup wrote:
>> "address@hidden" <address@hidden> writes:
> ...
>> > Did 104f80daf1dab11ef5b598006e3d4be8dfbe1926 go through full review?
>> > Was it verified with a full build?
>> 
>> Very likely.  This was not "gold star" quality which tends to work on
>> first compilation attempt.
>
> This patch looks absolutely excellent to me.
>
>   104f80daf1dab11ef5b598006e3d4be8dfbe1926
> fixed:
>   http://code.google.com/p/lilypond/issues/detail?id=1655
> first patch set May 17 or so:
>   http://codereview.appspot.com/4543055/
> next patch set May 26 or so:
>   http://codereview.appspot.com/4536068/
> this second patchset had a total of:
>   - 6 drafts
>   - 9 comments not from the author
>   - a LGTM from Neil
>   - countdowns on
>   June 03
>     http://lists.gnu.org/archive/html/lilypond-devel/2011-06/msg00036.html
>   June 06
>     http://lists.gnu.org/archive/html/lilypond-devel/2011-06/msg00104.html
>   June 14
>     http://lists.gnu.org/archive/html/lilypond-devel/2011-06/msg00283.html
>
> Speaking from an administrative perspective, this patch looked
> *excellent* to push.

The term "gold star" refers to code that runs on first compilation.  It
is not a measure of the final quality.  I was giving a subjective
impression of how "natural" the code appeared to me, and the code does
not exhibit the light and consistent hand of a master.  It looks like
the result of work rather than inspiration.  And that means that I am
fairly confident it _was_ verified with a full build.  Likely several
full builds before it compiled without errors.  It has a regression test
fitting it, to boot.

I agree with you that from an administrative perspective, this patch
could hardly have been better.  From my personal perspective as code
wrangler, there are a number of deficiencies in code and execution.
Probably worst is that the code does not speak for itself.  Often you
look at code, and see what it does and why.  Not here.

So it needs to tell its story in comments.  It doesn't.  There is a lot
of code in Lilypond that nonchalantly expects people to get along
without commenting what it does.  This is often a nuisance, but if the
code is written by a master, the pain of figuring out what it does is
usually tolerable.  I don't feel confident understanding what this patch
does and why.  Masters don't grow on trees.

So we have, in my view, a large discrepancy between administrative
quality and code quality for this patch.  And I want to _stress_ that
this is _not_, I repeat _not_ the fault of the patch author.  He
invested more care and effort than he sees us doing.

Can we do better?

-- 
David Kastrup




reply via email to

[Prev in Thread] Current Thread [Next in Thread]