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." <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 > mailto: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) > > > > > >
