db-derby-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Daniel John Debrunner <...@apache.org>
Subject Re: New contributors?
Date Thu, 07 Sep 2006 04:14:26 GMT
Susan L. Cline wrote:

[snip feedback on style discussion]
> I felt discouraged because I think if it is a style issue or it is a nit, then why
> does it need to be mentioned?  I'm not sure if others feel this way, but I can say
> personally, when submitting a patch (which I mentioned I have rarely done) I already
> a little bit nervous about it being coded correctly, without regard to style.

I think some of this depends on each person's definition of a "style
issue". Most of what might be called style issues I raise I believe have
to do with clarity of the resulting code. I believe this is important
for the long term clarity of the entire code base which I also believe
lowers the barrier for entry for new contributors. So I'm willing to not
 commit a technically correct patch that's full of confusing code or
lack of comments because it will lead to a harder to understand code
base that in the long run will be worse for the community. So I see that
as a trade-off between the short-term of making it easier for one
individual to get involved against making the long term easier for
multiple people to get involved.

As specific examples of issues that some may see as style, and I as clarity:

  - Logical naming of items (variables, fields, methods, classes etc.) I
really believe that this makes code much easier to read and understand,
especially for newcomers. Two examples are:
       *  when using a specific name would be clearer 'lengthInBytes'
rather than 'len' (e.g. is that length in characters or bytes?)
       * ensuring the name correctly refects the object's purpose. As a
crude example:

          public Time getBreakfastTime() {
               return lunchTime;
        People reading code that calls this method will see
'.getBreakfastTime()', thus assume the time being fetched is breakfast
time, thus leading to wrong assumptions and bugs.

   - Comments for methods and fields. Without comments it becomes really
hard for a reviewer or anyone reading the committed code to understand
the code, the problem really is that there is no guidance as to what the
contributor *intended* the functionality to be. The Java code in a
method may look ok, but is it what was intended or required? The human
readable comment describing the functionality (or intended purpose for a
field) really ties the intended behaviour to the code, and gives more
opportunity to discover bugs.

Maybe there's also an issue in the way we describe comment items: "nits"
&  "style issue", Kristian said item number 2) was a "nit", but look at
his comment "I had to spend a little time thinking about why you would
need that, until PI popped up in my head :) " - A comment to the method
would have meant he (and countless others) understood it without
spending time. Is that really a "nit", or is it useful feedback?

[snip - more background]

>>Since Apache is a peer based community, maybe it should be made clearer
>>that reviewers' comments are just "their comments", the contributor
>>doesn't have to follow all, or even any, of the advice. Any committer
>>can commit any patch that they "have a high degree of confidence in".
>>In my opinion a committer may gain confidence in a patch by requesting
>>certain changes in it through review comments. If those changes don't
>>happen or partially happen, then committer can re-evaluate their
>>confidence level in the patch, possibly leading to committing the patch.
>>If the committer decides not to commit the patch, then it's possible
>>they may modify it to have confidence in it and then submit it, though
>>this might occur much later when the committer (or anyone else) has time.
> This makes sense to me, and in a way is part of the reason I am confused
> by the process.  After your gave me your comments I believe I addressed all of
> them, even the "style" comments.  However, then I received a second set
> of comments from another person which were different from yours. 

Different, but not contradictory. I think that's a fact of open source,
or any code reviewing, different people see different things, have
different hot buttons based upon past experience. It's also the value of
the "multiple eyes".

> This is a somewhat rhetorical question, but should it be the responsibility of the 
> contributor to address all the comments of different committers? I would imagine this
> boils down to the desire the contributor has to getting his or her patch committed,
> which again, I see as a barrier to contribution.

The contributor is in the best position to address comments from
reviewers, thus it makes most sense for them to do it. However, the
comments should be seen as that, not "action items" that are required
for a patch. A contributor should be free to challenge or discuss any
comment, rather than assume the reviewer is correct. On some of the
style issues I raised, related to clarity, I did ask the question "do
they add value?". By "add value", I really meant does it increase the
clarity of the code? In such cases a contributor is free to come back
and say in their opinion it does and why, and maybe the reviewer and
community learns something as well, it's a two way street.
It's also valid for the contributor to say, those are great ideas, I
don't have time to address them now. Then it's up to the committers to
decide if one of them has the confidence in the patch.
Another interesting point is that any reviewer or committer looking at
the patch do not own it in any way, while some discussion is going on
between a reviewers and or committers and a contributor about a patch,
it's fine to another committer to say enough and just commit it.

Interesting comment about "barrier to contribution" and "desire to get a
a patch committed". To grow a community we need contributors that are
willing to follow the Apache way, work with the community to a common
consensus, thus resolve and discuss reviewers' comments. There's also a
place for "hit & run" contributors, who provide patches in any state and
disappear, but those contributors are not going to grow the community.

Thanks for bringing this issue out for discussion,

View raw message