accumulo-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Dave Marion <>
Subject Re: [DISCUSS] Pull Request Guidelines
Date Mon, 05 Jun 2017 15:44:42 GMT
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." <> 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 <
> 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)
> > 
> >     > 

  • Unnamed multipart/alternative (inline, None, 0 bytes)
View raw message