accumulo-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Keith Turner <ke...@deenlo.com>
Subject Re: [DISCUSS] Pull Request Guidelines
Date Mon, 05 Jun 2017 16:07:41 GMT
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
View raw message