accumulo-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Mike Miller <>
Subject Re: [DISCUSS] Pull Request Guidelines
Date Mon, 05 Jun 2017 17:19:52 GMT
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,
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 <> 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 <> 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." <> 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 <
> wrote:
>> >>
>> >> > > Sometimes I use review comments to just ask questions about things
>> >>> 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
>> >>> 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 < > 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)
>> >>>
>> >>> >
>> >>
>> >
>> >

View raw message