accumulo-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Sean Busbey <bus...@cloudera.com>
Subject Re: [DISCUSS] Pull Request Guidelines
Date Mon, 05 Jun 2017 21:47:57 GMT
IIRC Josh did some work towards this end back at the start of 2016.

I haven't checked on the current state of it in a long time though:

https://builds.apache.org/job/PreCommit-ACCUMULO-Build/

On Mon, Jun 5, 2017 at 1:00 PM, Mike Drob <madrob@cloudera.com> wrote:
> For what will be checked, maybe we ask nicely that somebody hook us in to
> Apache Yetus and get a "standard" suite of checks for free, complete with
> automated feedback.
>
> On Mon, Jun 5, 2017 at 12:54 PM, Dave Marion <dlmarion@comcast.net> wrote:
>
>> I have used Hadoop's documentation on this subject for submitting patches.
>> I'm not suggesting that we go to this level of detail, but as a new
>> contributor I know how to set up my IDE, what commands to run to create my
>> patch, and I know the items that are going to be checked at the start.
>>
>>
>> [1] https://wiki.apache.org/hadoop/HowToContribute
>>
>> [2] https://wiki.apache.org/hadoop/CodeReviewChecklist
>>
>>
>> >
>> >     On June 5, 2017 at 1:19 PM Mike Miller <michaelpmiller@gmail.com>
>> wrote:
>> >
>> >     I could be wrong, but it sounds like there are two different
>> >     perspectives being discussed here and it may be helpful to try and
>> >     separate the two. On one hand there are discussions of guidelines for
>> >     reviewers (Dave's initial list, Keith's ideas) to follow and on the
>> >     other hand, suggestions for contributors, which Christopher's list
>> >     sounds more geared towards. Since everyone on this list has to wear
>> >     both hats, I think each different point of view could benefit from
>> >     some loose guidelines.
>> >
>> >     For example, General Pull Request Guidelines for the Accumulo
>> community:
>> >     When submitting a PR... please run these commands [...] before
>> >     submitting to ensure code adheres to checkstyle and passes findbugs,
>> >     etc
>> >     When reviewing a PR... ensure dialog portrays how strongly the
>> >     reviewer feels about the comment [Could = optional suggestion, Should
>> >     = would be helpful but not blocking, Must = required]
>> >
>> >     On Mon, Jun 5, 2017 at 12:57 PM, Dave Marion <dlmarion@comcast.net>
>> wrote:
>> >
>> >         > >
>> > >         I think things can be improved when it comes to handling pull
>> requests. The point of this thread was to try and come up with something to
>> set expectations for the contributor. I figured the discussion would lead
>> to the modification of the existing example or to a new example.
>> Christopher provided a different example, but most of the feedback seemed
>> to indicate that this was not warranted. I'm not sure what else I can say
>> on the matter. If the majority thinks that its not a problem, then its not
>> a problem.
>> > >
>> > >             > > >
>> > > >             On June 5, 2017 at 12:39 PM Josh Elser <
>> josh.elser@gmail.com> wrote:
>> > > >
>> > > >             Perhaps this discussion would be better served if you
>> gave some concrete
>> > > >             suggestions on how you think things can/should be
>> improved.
>> > > >
>> > > >             e.g. Mike's suggestion of using the
>> maven-checkstyle-plugin earlier, why
>> > > >             not focus on that? Does this (still) work with the
>> build? If so, how do
>> > > >             we get that run automagically via travis or jenkins?
>> > > >
>> > > >             To me, it seems like you either wanted to throw some
>> shade or you are
>> > > >             genuinely concerned about a problem that others are not
>> (yet?) concerned
>> > > >             about. I doubt re-focusing contribution processes for
>> efficiency would
>> > > >             be met with disapproval.
>> > > >
>> > > >             On 6/5/17 12:32 PM, Dave Marion wrote:
>> > > >
>> > > >                 > > > >
>> > > > >                 The main entrance to the community for new
>> contributors is through pull requests. I have seen PR's approved in an
>> inconsistent manner. My intent was to make known the expectations for new
>> contributions so that newcomers don't get discouraged by the amount of
>> feedback and/or changes requested while providing some guidelines to make
>> it more consistent. It seems that there is not a desire to do this for
>> various reasons. That's fine by me and I'm willing to drop the discussion
>> here.
>> > > > >
>> > > > >                     > > > > >
>> > > > > >                     On June 5, 2017 at 12:14 PM "Marc P."
<
>> marc.parisi@gmail.com> wrote:
>> > > > > >
>> > > > > >                     Turner and Tubbs,
>> > > > > >                     You both piqued my interest and I agree.
>> There's something important in what both said regarding the discussion and
>> importance of a particular change. Style changes most likely aren't deal
>> breakers unless it is terribly confusing, but I would leave that up to the
>> reviewer and developer to discuss.
>> > > > > >
>> > > > > >                     Dave,
>> > > > > >                     I'm sure your intent is good and you
goal
>> isn't the handcuff reviewers. Is your concern over a stalemate on something
>> such as a code style? Would a discussion not be the remedy for this?
>> > > > > >
>> > > > > >                     On Mon, Jun 5, 2017 at 12:07 PM, Keith
>> Turner <keith@deenlo.com mailto:keith@deenlo.com > wrote:
>> > > > > >
>> > > > > >                         > > > > > >
>> > > > > > >                             > > > > >
> >
>> > > > > > > >                             Sometimes I use review
comments
>> to just ask questions about things I
>> > > > > > > >                             don't understand.
Sometimes when
>> looking at a code review, I have a
>> > > > > > > >                             thought about the
change that I
>> know is a subjective opinion. In this
>> > > > > > > >                             case I want to share
my thought,
>> in case they find it useful.
>> > > > > > > >                             However, I don't care
if a
>> change is made or not. Sometimes I think a
>> > > > > > > >                             change must be made.
I try to
>> communicate my intentions, but its
>> > > > > > > >                             wordy, slow, and I
don't think I
>> always succeed.
>> > > > > > > >
>> > > > > > > >                         > > > > >
>
>> > > > > > >                         Given there are so many ways
the
>> comments on a review can be used, I
>> > > > > > >                         think it can be difficult to
quickly
>> know the intentions of the
>> > > > > > >                         reviewer. I liked review board's
>> issues, I think they helped with
>> > > > > > >                         this problem. A reviewer could
make
>> comments and issues. The issues
>> > > > > > >                         made it clear what the reviewer
>> thought must be done vs discussion.
>> > > > > > >                         Issues made reviews more efficient
by
>> making the intentions clear AND
>> > > > > > >                         separating important concerns
from
>> lots of discussion.
>> > > > > > >
>> > > > > > >                         When I submit a PR and it has
lots of
>> comments, towards the end I go
>> > > > > > >                         back and look through all of
the
>> comments to make sure I didn't miss
>> > > > > > >                         anything important. Its annoying
to
>> have to do this. Is there
>> > > > > > >                         anything we could do in GH
to
>> replicate this and help separate the
>> > > > > > >                         signal from the noise?
>> > > > > > >
>> > > > > > >                         On Mon, Jun 5, 2017 at 11:08
AM, Dave
>> Marion <dlmarion@comcast.net mailto:dlmarion@comcast.net > wrote:
>> > > > > > >
>> > > > > > >                             > > > > >
> >
>> > > > > > > >                             I propose that we
define a set
>> of guidelines to use when reviewing pull requests. In doing so,
>> contributors will be able to determine potential issues in their code
>> possibly reducing the number of changes that occur before acceptance.
>> Here's an example to start the discussion:
>> > > > > > > >
>> > > > > > > >                             Items a reviewer should
look for:
>> > > > > > > >
>> > > > > > > >                                1. Adherence to
code
>> formatting rules (link to formatting rules)
>> > > > > > > >
>> > > > > > > >                                2. Unit tests required
>> > > > > > > >
>> > > > > > > >                                3. Threading issues
>> > > > > > > >
>> > > > > > > >                                4. Performance
implications
>> > > > > > > >
>> > > > > > > >                             Items that should
not block
>> acceptance:
>> > > > > > > >
>> > > > > > > >                                1. Stylistic changes
that
>> have no performance benefit
>> > > > > > > >
>> > > > > > > >                                2. Addition of
features
>> outside the scope of the ticket (moving the goal post, discussion should
>> lead to ticket creation)
>> > > > > > > >
>> > > > > > > >                         > > > > >
>
>> > > > > > >                         >
>> > > > > > >
>> > > > > > >                     > > > > >
>> > > > > >                 > > > >
>> > > > >             > > >
>> > > >         > >
>> > >     >
>>



-- 
busbey

Mime
View raw message