> 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. <marc.par...@gmail.com> 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 <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) > > >
