accumulo-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Mike Drob <mad...@cloudera.com>
Subject Re: [DISCUSS] Pull Request Guidelines
Date Mon, 05 Jun 2017 18:00:32 GMT
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)
> > > > > > > >
> > > > > > > >                         > > > > > >
> > > > > > >                         >
> > > > > > >
> > > > > > >                     > > > > >
> > > > > >                 > > > >
> > > > >             > > >
> > > >         > >
> > >     >
>

Mime
  • Unnamed multipart/alternative (inline, None, 0 bytes)
View raw message