Valik wrote:
>
> I want to add in my observations to this as well. I've been reading the
> list for a few days now and I'm just astounded at the process it takes
> to get a patch committed. As an outsider looking in, your methods are
> very draconian. I can't recall the number of patch threads I've read in
> the last couple days but I walk away from every one of them with the
> same thought in mind: These guys critique more on "ugly" code than they
> do on poorly written code.
[...]
Thank you for your frank and well written observations, Mark and Valik. I
think you must be right in your assessment that many of our responses to
patches from "outsiders" or new contributors are badly focused. I am guilty of
this. I know that I tend to comment first on the easiest things to spot, which
tend to be style issues* and typos. I hadn't really stopped to think how
harmful this attitude could be.
When responding to an experienced committer, bluntly pointing out a list of
style problems is not nearly so harmful (though still not ideal), because they
get the style nearly right first time, consider it their job to maintain the
style anyway, and are unlikely to be put off by such feedback because they know
why it's important and also know that their code is actually being reviewed for
content as well, even if the first response doesn't mention it.
Therefore the important issue here is to ensure we respond to new contributors
in a way that encourages them.
Let us not forget that it is genuinely helpful to the project if a contributor
does take care of the style. Just to give a little bit of justification for
fussing so much about style, I actually find it significantly hard to ignore
little inconsistencies when reading code. My brain is trained to spot them,
because it's an essential part of programming. Therefore I can review the
technical content more easily if the style is good. However, as you point out,
this consideration should take second place to the functional content of the patch.
I think I have been placing almost as much emphasis on getting the style right
as getting the content right, and I agree that that is the wrong thing to do,
and that we should be prepared to adjust it ourselves if a contributor wants to
provide a useful patch without bothering much about the style. Of course we do
that to some extent already, but it would be beneficial to do so to a
considerably greater extent.
What I Think I Should Do
Make an effort to identify whether I'm speaking to a relatively new
contributor, and respond differently if so.
Ignore or be very gentle about style at first, postponing criticisms until the
contributor shows a willingness to help us further by addressing style issues.
Even then it should be made clear that the extra help is optional.
When a patch has only style issues remaining, say so, and say then that it will
be accepted even if the contributor does not wish to fix them.
Say to what extent I have reviewed and found the other, uncriticised aspects of
the patch to be good. Sometimes I just make some remarks and the contributor
is left wondering whether fixing those is all he needs to do or they are just
the tip of the iceberg.
I quickly tire of writing the same general advice and encouragement in one
review after another, so I tend not to bother, but I should do because it's not
necessarily obvious to the contributor.
If there are many style issues to address, I should ask the contributor to read
a part of the "Hacking" guide rather than just pointing out each little violation.
Revise the "Hacking" guide to separate the basic guidelines for newcomers from
the more detailed information, so that the basic guide is less daunting.
- Julian
[* By "style" in this message I mean low-level issues like source layout,
comments, and the naming of variables. By "content" I mean the high-level
design and functionality.]
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Received on Mon Jul 25 15:08:03 2005