accumulo-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Josh Elser <josh.el...@gmail.com>
Subject Re: [DISCUSS] Pull Request Guidelines
Date Mon, 05 Jun 2017 22:01:52 GMT
Hah, isn't that ironic. I didn't even remember starting that work.

Now that you mention it, I remember running into some issue in Yetus, 
but I think that was fixed upstream.

On 6/5/17 5:47 PM, Sean Busbey wrote:
> 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)
>>>>>>>>>>
>>>>>>>>>>                          > > > > >
>
>>>>>>>>>                          >
>>>>>>>>>
>>>>>>>>>                      > > > > >
>>>>>>>>                  > > > >
>>>>>>>              > > >
>>>>>>          > >
>>>>>      >
>>>
> 
> 
> 

Mime
View raw message