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 <michaelpmil...@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 <dlmar...@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.el...@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.par...@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 <ke...@deenlo.com mailto:ke...@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 <dlmar...@comcast.net mailto:dlmar...@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)
> > > > > >
>
> > > > >
> > > >
> > >
> >
>
