Mailing-List: contact dev-help@accumulo.apache.org; run by ezmlm
Precedence: bulk
Reply-To: dev@accumulo.apache.org
MIME-Version: 1.0
In-Reply-To: <CALs6WttCuQ3piM2DYobfCJdwuNPjsUg9xFknTXCnL4v2qw_BCg@mail.gmail.com>
References: <1263150021.90725.1496675313506@connect.xfinity.com>
 <CAGUtCHrPf2XGm920W=C=x=K9mWm19Yugm632E1QA8Z7bfb_Yqw@mail.gmail.com>
 <CAH0f5v2MCpCSLrKos41aoeLC6OezY-U3vg=sJQ4NBQgM5LqTsQ@mail.gmail.com>
 <1216526815.96005.1496680343808@connect.xfinity.com> <85a7cc91-fcbb-10cf-fadf-526614154897@gmail.com>
 <1194646934.97271.1496681853617@connect.xfinity.com> <CALs6WttCuQ3piM2DYobfCJdwuNPjsUg9xFknTXCnL4v2qw_BCg@mail.gmail.com>
From: Keith Turner <keith@deenlo.com>
Date: Mon, 5 Jun 2017 14:01:29 -0400
Message-ID: <CAGUtCHpbk=dadwkX=YJS4pc1T=pXUJDD_Q6D+tAU0KrvchbpZw@mail.gmail.com>
Subject: Re: [DISCUSS] Pull Request Guidelines
To: Accumulo Dev List <dev@accumulo.apache.org>
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable
archived-at: Mon, 05 Jun 2017 18:01:36 -0000

If Accumulo had a CONTRIBUTING.md, then when someone opens a PR GH
would offer a link to it.

https://help.github.com/articles/helping-people-contribute-to-your-project/

On Mon, Jun 5, 2017 at 1:19 PM, Mike Miller <michaelpmiller@gmail.com> wrot=
e:
> 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 =3D optional suggestion, Should
> =3D would be helpful but not blocking, Must =3D 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 expec=
tations for the contributor. I figured the discussion would lead to the mod=
ification 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 concret=
e
>>> suggestions on how you think things can/should be improved.
>>>
>>> e.g. Mike's suggestion of using the maven-checkstyle-plugin earlier, wh=
y
>>> 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?) concerne=
d
>>> 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 pu=
ll 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 w=
hile providing some guidelines to make it more consistent. It seems that th=
ere 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 terribl=
y confusing, but I would leave that up to the reviewer and developer to dis=
cuss.
>>> >>
>>> >> Dave,
>>> >> I'm sure your intent is good and you goal isn't the handcuff reviewe=
rs. Is your concern over a stalemate on something such as a code style? Wou=
ld a discussion not be the remedy for this?
>>> >>
>>> >> On Mon, Jun 5, 2017 at 12:07 PM, Keith Turner <keith@deenlo.com mail=
to:keith@deenlo.com > wrote:
>>> >>
>>> >> > > Sometimes I use review comments to just ask questions about thin=
gs 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 th=
is
>>> >>> 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 A=
ND
>>> >>> separating important concerns from lots of discussion.
>>> >>>
>>> >>> When I submit a PR and it has lots of comments, towards the end I g=
o
>>> >>> back and look through all of the comments to make sure I didn't mis=
s
>>> >>> 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 reviewin=
g pull requests. In doing so, contributors will be able to determine potent=
ial 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 t=
he goal post, discussion should lead to ticket creation)
>>> >>>
>>> >>> >
>>> >>
>>> >
>>> >