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 15:25:07 GMT
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 <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