[DISCUSS] Pull Request Guidelines

classic Classic list List threaded Threaded
22 messages Options
12
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

[DISCUSS] Pull Request Guidelines

Dave Marion-2
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)
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: [DISCUSS] Pull Request Guidelines

Tony Kurc
Dave, on your not block acceptance #1 - where would something like ensuring
consistent "look and feel" of APIs fit? I recently had a PR for another
project and recommended a class name change to something more consistent
with the rest of the project.


On Mon, Jun 5, 2017 at 11:08 AM, Dave Marion <[hidden email]> 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)
>
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: [DISCUSS] Pull Request Guidelines

Dave Marion-2
I think that changes to the public API would be under more scrutiny and hopefully a consensus could be reached. I don't think we have hard and fast rules for our API conventions.

> On June 5, 2017 at 11:19 AM Tony Kurc <[hidden email]> wrote:
>
>
> Dave, on your not block acceptance #1 - where would something like ensuring
> consistent "look and feel" of APIs fit? I recently had a PR for another
> project and recommended a class name change to something more consistent
> with the rest of the project.
>
>
> On Mon, Jun 5, 2017 at 11:08 AM, Dave Marion <[hidden email]> 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)
> >
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: [DISCUSS] Pull Request Guidelines

Marc P.
In reply to this post by Dave Marion-2
Dave,
  I don't agree that stylistic changes are something to ignore. There may
be cases where something is confusing to others and thus should be called
out. This is difficult to blatantly avoid.

  I can't agree with number two either since a PR can be a form of
requirements elicitation and such there are cases in which there are new
preconditions on the ticket. While your "not block of acceptance" may
sometimes apply I don't think it goes to fitting a community of developers,
where you can discuss your differences. In the case of number one and two
developers reviewing will pick their battles and perhaps other reviewers
can chime in on the importance of said feature. What is the purpose of
limiting this discussion my claiming it cannot impact acceptance?

  Bad code begets bad code and if a developer wants to take issue with
code, they should be allowed to discuss this within the PR. Further,
inconsistency begets inconsistency, so wild departures from the norm should
be something a reviewer has the levity to discuss.

  While discussion should lead to ticket creation we should avoid creating
features that need a portion completed to be used in production
successfully.




On Mon, Jun 5, 2017 at 11:08 AM, Dave Marion <[hidden email]> 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)
>
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: [DISCUSS] Pull Request Guidelines

Mike Drob-4
> 1. Adherence to code formatting rules (link to formatting rules)

Can we let checkstyle handle this instead of humans worrying about it?

On Mon, Jun 5, 2017 at 10:25 AM, Marc P. <[hidden email]> wrote:

> Dave,
>   I don't agree that stylistic changes are something to ignore. There may
> be cases where something is confusing to others and thus should be called
> out. This is difficult to blatantly avoid.
>
>   I can't agree with number two either since a PR can be a form of
> requirements elicitation and such there are cases in which there are new
> preconditions on the ticket. While your "not block of acceptance" may
> sometimes apply I don't think it goes to fitting a community of developers,
> where you can discuss your differences. In the case of number one and two
> developers reviewing will pick their battles and perhaps other reviewers
> can chime in on the importance of said feature. What is the purpose of
> limiting this discussion my claiming it cannot impact acceptance?
>
>   Bad code begets bad code and if a developer wants to take issue with
> code, they should be allowed to discuss this within the PR. Further,
> inconsistency begets inconsistency, so wild departures from the norm should
> be something a reviewer has the levity to discuss.
>
>   While discussion should lead to ticket creation we should avoid creating
> features that need a portion completed to be used in production
> successfully.
>
>
>
>
> On Mon, Jun 5, 2017 at 11:08 AM, Dave Marion <[hidden email]> 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)
> >
>
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: [DISCUSS] Pull Request Guidelines

Dave Marion-2
In reply to this post by Marc P.
I'm not suggesting that stylistic changes should be ignored; in the example I was suggesting they should not be a blocker. The reviewer certainly should ask questions to understand the code, and suggest changes to make things clearer. Regarding #2, I agree that some PR's may have to wait to be merged until another issue is resolved.

I'm not trying to handcuff reviewers here, I'm just proposing that we handle PR's with some consistency. I fully agree that the reviewer should be able to voice his/her opinions. However, it would be good if the acceptance bar was close to the same for all contributions. I personally have been burned and totally put off by a contribution I was trying to make another open source project. I think that if there is a (somewhat) loose, but defined set of expectations on both sides of the contribution, it might be a better experience.


> On June 5, 2017 at 11:25 AM "Marc P." <[hidden email]> wrote:
>
>     Dave,
>       I don't agree that stylistic changes are something to ignore. There may be cases where something is confusing to others and thus should be called out. This is difficult to blatantly avoid.
>
>       I can't agree with number two either since a PR can be a form of requirements elicitation and such there are cases in which there are new preconditions on the ticket. While your "not block of acceptance" may sometimes apply I don't think it goes to fitting a community of developers, where you can discuss your differences. In the case of number one and two developers reviewing will pick their battles and perhaps other reviewers can chime in on the importance of said feature. What is the purpose of limiting this discussion my claiming it cannot impact acceptance?
>
>       Bad code begets bad code and if a developer wants to take issue with code, they should be allowed to discuss this within the PR. Further, inconsistency begets inconsistency, so wild departures from the norm should be something a reviewer has the levity to discuss.
>
>       While discussion should lead to ticket creation we should avoid creating features that need a portion completed to be used in production successfully.
>
>        
>
>
>     On Mon, Jun 5, 2017 at 11:08 AM, Dave Marion <[hidden email] mailto:[hidden email] > 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)
> >
> >     >
>
 
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: [DISCUSS] Pull Request Guidelines

Dave Marion-2
In reply to this post by Mike Drob-4
That's the intent. If we have some guidelines, and they are read first by a new contributor, then they will know that their is a formatter that they should be using.

> On June 5, 2017 at 11:35 AM Mike Drob <[hidden email]> wrote:
>
>
> > 1. Adherence to code formatting rules (link to formatting rules)
>
> Can we let checkstyle handle this instead of humans worrying about it?
>
> On Mon, Jun 5, 2017 at 10:25 AM, Marc P. <[hidden email]> wrote:
>
> > Dave,
> > I don't agree that stylistic changes are something to ignore. There may
> > be cases where something is confusing to others and thus should be called
> > out. This is difficult to blatantly avoid.
> >
> > I can't agree with number two either since a PR can be a form of
> > requirements elicitation and such there are cases in which there are new
> > preconditions on the ticket. While your "not block of acceptance" may
> > sometimes apply I don't think it goes to fitting a community of developers,
> > where you can discuss your differences. In the case of number one and two
> > developers reviewing will pick their battles and perhaps other reviewers
> > can chime in on the importance of said feature. What is the purpose of
> > limiting this discussion my claiming it cannot impact acceptance?
> >
> > Bad code begets bad code and if a developer wants to take issue with
> > code, they should be allowed to discuss this within the PR. Further,
> > inconsistency begets inconsistency, so wild departures from the norm should
> > be something a reviewer has the levity to discuss.
> >
> > While discussion should lead to ticket creation we should avoid creating
> > features that need a portion completed to be used in production
> > successfully.
> >
> >
> >
> >
> > On Mon, Jun 5, 2017 at 11:08 AM, Dave Marion <[hidden email]> 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)
> > >
> >
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: [DISCUSS] Pull Request Guidelines

Christopher Tubbs-2
In reply to this post by Marc P.
I'm not entirely sure what such specific guidelines hope to achieve. I'm in
favor of having some "things to look for" guidelines, but I don't think
strict guidelines are going to work, because it elevates rules over the
human element. I don't think we can be too prescriptive about what things a
contributor *should* do, or what a reviewer *should* check for, because
everybody has unique volunteer time constraints, expertise, and passions.

I'm also concerned about long-term maintenance of any such documentation,
which seems likely to quickly get out of date as the community itself
evolves, if we are too specific. Any such guidelines should be maintainable
by being succinct, generic, and flexible. I'm thinking something like:

1. Follow the ASF Code of Conduct [link]
2. Ensure build passes (captures most checks; can rely on Travis CI)
[suggested build command-line]
3. Check for style and formatting failures, or uncommitted changes from the
build tooling after a build
5. Consider semver rules [link], especially when contributing to
maintenance branches
6. Add tests when appropriate
7. Be willing to discuss performance impact, API design, style choices

The key thing about contributing and reviewing is being willing to engage
in polite discussion about any part of the contribution. Too much
specificity will result in docs which don't apply to majority of
circumstances, or docs which become out-of-date quickly, or docs which
nobody reads because they are too complicated or long. The upfront costs of
trying to make sure you comply with all possible guidelines may also be a
deterrence from contributing. It's much easier to be willing to receive
friendly feedback than it is to try to make sure you take care of any
possible thing somebody might result in a change suggestion.

On Mon, Jun 5, 2017 at 11:25 AM Marc P. <[hidden email]> wrote:

> Dave,
>   I don't agree that stylistic changes are something to ignore. There may
> be cases where something is confusing to others and thus should be called
> out. This is difficult to blatantly avoid.
>
>   I can't agree with number two either since a PR can be a form of
> requirements elicitation and such there are cases in which there are new
> preconditions on the ticket. While your "not block of acceptance" may
> sometimes apply I don't think it goes to fitting a community of developers,
> where you can discuss your differences. In the case of number one and two
> developers reviewing will pick their battles and perhaps other reviewers
> can chime in on the importance of said feature. What is the purpose of
> limiting this discussion my claiming it cannot impact acceptance?
>
>   Bad code begets bad code and if a developer wants to take issue with
> code, they should be allowed to discuss this within the PR. Further,
> inconsistency begets inconsistency, so wild departures from the norm should
> be something a reviewer has the levity to discuss.
>
>   While discussion should lead to ticket creation we should avoid creating
> features that need a portion completed to be used in production
> successfully.
>
>
>
>
> On Mon, Jun 5, 2017 at 11:08 AM, Dave Marion <[hidden email]> 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)
> >
>
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: [DISCUSS] Pull Request Guidelines

Keith Turner
In reply to this post by Dave Marion-2
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 <[hidden email]> 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)
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: [DISCUSS] Pull Request Guidelines

Marc P.
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 <[hidden email]> 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 <[hidden email]> 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)
>
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: [DISCUSS] Pull Request Guidelines

Dave Marion-2
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." <[hidden email]> 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 <[hidden email] mailto:[hidden email] > 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 <[hidden email] mailto:[hidden email] > 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)
> >
> >     >
>
 
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: [DISCUSS] Pull Request Guidelines

Josh Elser
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." <[hidden email]> 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 <[hidden email] mailto:[hidden email] > 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 <[hidden email] mailto:[hidden email] > 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)
>>>
>>>      >
>>
>  
>
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: [DISCUSS] Pull Request Guidelines

Mike Walch-2
In reply to this post by Christopher Tubbs-2
I agree with Christopher about avoiding strict guidelines for reviews but
having some loose guidelines/advice for reviewers.

One thing that I have found helpful is when reviewers communicate how
strongly they feel about a change:

- If the change is optional:       Could add a unit test for this class
- If the change is suggested:   A unit test should be added for this class
- If the change is required:       I am -1 on this PR until a unit test is
added for this class.

As a reviewer, I really like using "Could" before my PR comments.  It lets
me suggest whatever change I want without forcing the contributor to make
these changes if they disagree.

On Mon, Jun 5, 2017 at 12:00 PM Christopher <[hidden email]> wrote:

> I'm not entirely sure what such specific guidelines hope to achieve. I'm in
> favor of having some "things to look for" guidelines, but I don't think
> strict guidelines are going to work, because it elevates rules over the
> human element. I don't think we can be too prescriptive about what things a
> contributor *should* do, or what a reviewer *should* check for, because
> everybody has unique volunteer time constraints, expertise, and passions.
>
> I'm also concerned about long-term maintenance of any such documentation,
> which seems likely to quickly get out of date as the community itself
> evolves, if we are too specific. Any such guidelines should be maintainable
> by being succinct, generic, and flexible. I'm thinking something like:
>
> 1. Follow the ASF Code of Conduct [link]
> 2. Ensure build passes (captures most checks; can rely on Travis CI)
> [suggested build command-line]
> 3. Check for style and formatting failures, or uncommitted changes from the
> build tooling after a build
> 5. Consider semver rules [link], especially when contributing to
> maintenance branches
> 6. Add tests when appropriate
> 7. Be willing to discuss performance impact, API design, style choices
>
> The key thing about contributing and reviewing is being willing to engage
> in polite discussion about any part of the contribution. Too much
> specificity will result in docs which don't apply to majority of
> circumstances, or docs which become out-of-date quickly, or docs which
> nobody reads because they are too complicated or long. The upfront costs of
> trying to make sure you comply with all possible guidelines may also be a
> deterrence from contributing. It's much easier to be willing to receive
> friendly feedback than it is to try to make sure you take care of any
> possible thing somebody might result in a change suggestion.
>
> On Mon, Jun 5, 2017 at 11:25 AM Marc P. <[hidden email]> wrote:
>
> > Dave,
> >   I don't agree that stylistic changes are something to ignore. There may
> > be cases where something is confusing to others and thus should be called
> > out. This is difficult to blatantly avoid.
> >
> >   I can't agree with number two either since a PR can be a form of
> > requirements elicitation and such there are cases in which there are new
> > preconditions on the ticket. While your "not block of acceptance" may
> > sometimes apply I don't think it goes to fitting a community of
> developers,
> > where you can discuss your differences. In the case of number one and two
> > developers reviewing will pick their battles and perhaps other reviewers
> > can chime in on the importance of said feature. What is the purpose of
> > limiting this discussion my claiming it cannot impact acceptance?
> >
> >   Bad code begets bad code and if a developer wants to take issue with
> > code, they should be allowed to discuss this within the PR. Further,
> > inconsistency begets inconsistency, so wild departures from the norm
> should
> > be something a reviewer has the levity to discuss.
> >
> >   While discussion should lead to ticket creation we should avoid
> creating
> > features that need a portion completed to be used in production
> > successfully.
> >
> >
> >
> >
> > On Mon, Jun 5, 2017 at 11:08 AM, Dave Marion <[hidden email]>
> 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)
> > >
> >
>
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: [DISCUSS] Pull Request Guidelines

Christopher Tubbs-2
I think consistency is a good thing to strive for, but I think
inconsistency is unavoidable, especially in an all-volunteer community and
where each contribution is unique. I like Mike Walch's suggestion about how
to clearly phrase things. I think recommended phrasing might make for good
guidelines on how to perform a review.

On Mon, Jun 5, 2017 at 12:41 PM Mike Walch <[hidden email]> wrote:

> I agree with Christopher about avoiding strict guidelines for reviews but
> having some loose guidelines/advice for reviewers.
>
> One thing that I have found helpful is when reviewers communicate how
> strongly they feel about a change:
>
> - If the change is optional:       Could add a unit test for this class
> - If the change is suggested:   A unit test should be added for this class
> - If the change is required:       I am -1 on this PR until a unit test is
> added for this class.
>
> As a reviewer, I really like using "Could" before my PR comments.  It lets
> me suggest whatever change I want without forcing the contributor to make
> these changes if they disagree.
>
> On Mon, Jun 5, 2017 at 12:00 PM Christopher <[hidden email]> wrote:
>
> > I'm not entirely sure what such specific guidelines hope to achieve. I'm
> in
> > favor of having some "things to look for" guidelines, but I don't think
> > strict guidelines are going to work, because it elevates rules over the
> > human element. I don't think we can be too prescriptive about what
> things a
> > contributor *should* do, or what a reviewer *should* check for, because
> > everybody has unique volunteer time constraints, expertise, and passions.
> >
> > I'm also concerned about long-term maintenance of any such documentation,
> > which seems likely to quickly get out of date as the community itself
> > evolves, if we are too specific. Any such guidelines should be
> maintainable
> > by being succinct, generic, and flexible. I'm thinking something like:
> >
> > 1. Follow the ASF Code of Conduct [link]
> > 2. Ensure build passes (captures most checks; can rely on Travis CI)
> > [suggested build command-line]
> > 3. Check for style and formatting failures, or uncommitted changes from
> the
> > build tooling after a build
> > 5. Consider semver rules [link], especially when contributing to
> > maintenance branches
> > 6. Add tests when appropriate
> > 7. Be willing to discuss performance impact, API design, style choices
> >
> > The key thing about contributing and reviewing is being willing to engage
> > in polite discussion about any part of the contribution. Too much
> > specificity will result in docs which don't apply to majority of
> > circumstances, or docs which become out-of-date quickly, or docs which
> > nobody reads because they are too complicated or long. The upfront costs
> of
> > trying to make sure you comply with all possible guidelines may also be a
> > deterrence from contributing. It's much easier to be willing to receive
> > friendly feedback than it is to try to make sure you take care of any
> > possible thing somebody might result in a change suggestion.
> >
> > On Mon, Jun 5, 2017 at 11:25 AM Marc P. <[hidden email]> wrote:
> >
> > > Dave,
> > >   I don't agree that stylistic changes are something to ignore. There
> may
> > > be cases where something is confusing to others and thus should be
> called
> > > out. This is difficult to blatantly avoid.
> > >
> > >   I can't agree with number two either since a PR can be a form of
> > > requirements elicitation and such there are cases in which there are
> new
> > > preconditions on the ticket. While your "not block of acceptance" may
> > > sometimes apply I don't think it goes to fitting a community of
> > developers,
> > > where you can discuss your differences. In the case of number one and
> two
> > > developers reviewing will pick their battles and perhaps other
> reviewers
> > > can chime in on the importance of said feature. What is the purpose of
> > > limiting this discussion my claiming it cannot impact acceptance?
> > >
> > >   Bad code begets bad code and if a developer wants to take issue with
> > > code, they should be allowed to discuss this within the PR. Further,
> > > inconsistency begets inconsistency, so wild departures from the norm
> > should
> > > be something a reviewer has the levity to discuss.
> > >
> > >   While discussion should lead to ticket creation we should avoid
> > creating
> > > features that need a portion completed to be used in production
> > > successfully.
> > >
> > >
> > >
> > >
> > > On Mon, Jun 5, 2017 at 11:08 AM, Dave Marion <[hidden email]>
> > 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)
> > > >
> > >
> >
>
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: [DISCUSS] Pull Request Guidelines

Dave Marion-2
In reply to this post by Josh Elser
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 <[hidden email]> 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." <[hidden email]> 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 <[hidden email] mailto:[hidden email] > 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 <[hidden email] mailto:[hidden email] > 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)
> >>>
> >>> >
> >>
> >
> >
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: [DISCUSS] Pull Request Guidelines

Mike Miller
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 <[hidden email]> 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 <[hidden email]> 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." <[hidden email]> 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 <[hidden email] mailto:[hidden email] > 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 <[hidden email] mailto:[hidden email] > 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)
>> >>>
>> >>> >
>> >>
>> >
>> >
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: [DISCUSS] Pull Request Guidelines

Dave Marion-2
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 <[hidden email]> 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 <[hidden email]> 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 <[hidden email]> 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." <[hidden email]> 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 <[hidden email] mailto:[hidden email] > 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 <[hidden email] mailto:[hidden email] > 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)
> > > > > > >
> > > > > > >                         > > > > > >
> > > > > >                         >
> > > > > >
> > > > > >                     > > > > >
> > > > >                 > > > >
> > > >             > > >
> > >         > >
> >     >
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: [DISCUSS] Pull Request Guidelines

Mike Drob-3
For what will be checked, maybe we ask nicely that somebody hook us in to
Apache Yetus and get a "standard" suite of checks for free, complete with
automated feedback.

On Mon, Jun 5, 2017 at 12:54 PM, Dave Marion <[hidden email]> wrote:

> 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 <[hidden email]>
> 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 <[hidden email]>
> 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 <
> [hidden email]> 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." <
> [hidden email]> 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 <[hidden email] mailto:[hidden email] > 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 <[hidden email] mailto:[hidden email] > 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)
> > > > > > > >
> > > > > > > >                         > > > > > >
> > > > > > >                         >
> > > > > > >
> > > > > > >                     > > > > >
> > > > > >                 > > > >
> > > > >             > > >
> > > >         > >
> > >     >
>
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: [DISCUSS] Pull Request Guidelines

Keith Turner
In reply to this post by Mike Miller
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 <[hidden email]> 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 <[hidden email]> 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 <[hidden email]> 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." <[hidden email]> 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 <[hidden email] mailto:[hidden email] > 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 <[hidden email] mailto:[hidden email] > 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)
>>> >>>
>>> >>> >
>>> >>
>>> >
>>> >
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: [DISCUSS] Pull Request Guidelines

Christopher Tubbs-2
That would be great to have. A "What to expect" section in that could
contain stuff for reviewers, but framed for the contributor: "You can
expect to be asked about tests, style choices, performance, etc." So, it
would both serve to inform contributors, as well as act as a reference for
reviewers.

On Mon, Jun 5, 2017 at 2:01 PM Keith Turner <[hidden email]> wrote:

> 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 <[hidden email]>
> 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 <[hidden email]>
> 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 <[hidden email]> 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." <[hidden email]>
> 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 <[hidden email]
> mailto:[hidden email] > 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 <[hidden email]
> mailto:[hidden email] > 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)
> >>> >>>
> >>> >>> >
> >>> >>
> >>> >
> >>> >
>
12
Loading...