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 <dlmar...@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) >
