Turner and Tubbs, You both piqued my interest and I agree. There's something important in what both said regarding the discussion and importance of a particular change. Style changes most likely aren't deal breakers unless it is terribly confusing, but I would leave that up to the reviewer and developer to discuss.
Dave, I'm sure your intent is good and you goal isn't the handcuff reviewers. Is your concern over a stalemate on something such as a code style? Would a discussion not be the remedy for this? On Mon, Jun 5, 2017 at 12:07 PM, Keith Turner <ke...@deenlo.com> wrote: > Sometimes I use review comments to just ask questions about things I > don't understand. Sometimes when looking at a code review, I have a > thought about the change that I know is a subjective opinion. In this > case I want to share my thought, in case they find it useful. > However, I don't care if a change is made or not. Sometimes I think a > change must be made. I try to communicate my intentions, but its > wordy, slow, and I don't think I always succeed. > > Given there are so many ways the comments on a review can be used, I > think it can be difficult to quickly know the intentions of the > reviewer. I liked review board's issues, I think they helped with > this problem. A reviewer could make comments and issues. The issues > made it clear what the reviewer thought must be done vs discussion. > Issues made reviews more efficient by making the intentions clear AND > separating important concerns from lots of discussion. > > When I submit a PR and it has lots of comments, towards the end I go > back and look through all of the comments to make sure I didn't miss > anything important. Its annoying to have to do this. Is there > anything we could do in GH to replicate this and help separate the > signal from the noise? > > > 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) >
