accumulo-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Marc P." <marc.par...@gmail.com>
Subject Re: [DISCUSS] Pull Request Guidelines
Date Mon, 05 Jun 2017 16:14:33 GMT
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> 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> 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