That would be great to have. A "What to expect" section in that could contain stuff for reviewers, but framed for the contributor: "You can expect to be asked about tests, style choices, performance, etc." So, it would both serve to inform contributors, as well as act as a reference for reviewers.
On Mon, Jun 5, 2017 at 2:01 PM Keith Turner <ke...@deenlo.com> wrote: > If Accumulo had a CONTRIBUTING.md, then when someone opens a PR GH > would offer a link to it. > > https://help.github.com/articles/helping-people-contribute-to-your-project/ > > On Mon, Jun 5, 2017 at 1:19 PM, Mike Miller <michaelpmil...@gmail.com> > wrote: > > I could be wrong, but it sounds like there are two different > > perspectives being discussed here and it may be helpful to try and > > separate the two. On one hand there are discussions of guidelines for > > reviewers (Dave's initial list, Keith's ideas) to follow and on the > > other hand, suggestions for contributors, which Christopher's list > > sounds more geared towards. Since everyone on this list has to wear > > both hats, I think each different point of view could benefit from > > some loose guidelines. > > > > For example, General Pull Request Guidelines for the Accumulo community: > > When submitting a PR... please run these commands [...] before > > submitting to ensure code adheres to checkstyle and passes findbugs, > > etc > > When reviewing a PR... ensure dialog portrays how strongly the > > reviewer feels about the comment [Could = optional suggestion, Should > > = would be helpful but not blocking, Must = required] > > > > On Mon, Jun 5, 2017 at 12:57 PM, Dave Marion <dlmar...@comcast.net> > wrote: > >> I think things can be improved when it comes to handling pull requests. > The point of this thread was to try and come up with something to set > expectations for the contributor. I figured the discussion would lead to > the modification of the existing example or to a new example. Christopher > provided a different example, but most of the feedback seemed to indicate > that this was not warranted. I'm not sure what else I can say on the > matter. If the majority thinks that its not a problem, then its not a > problem. > >> > >>> On June 5, 2017 at 12:39 PM Josh Elser <josh.el...@gmail.com> wrote: > >>> > >>> > >>> Perhaps this discussion would be better served if you gave some > concrete > >>> suggestions on how you think things can/should be improved. > >>> > >>> e.g. Mike's suggestion of using the maven-checkstyle-plugin earlier, > why > >>> not focus on that? Does this (still) work with the build? If so, how do > >>> we get that run automagically via travis or jenkins? > >>> > >>> To me, it seems like you either wanted to throw some shade or you are > >>> genuinely concerned about a problem that others are not (yet?) > concerned > >>> about. I doubt re-focusing contribution processes for efficiency would > >>> be met with disapproval. > >>> > >>> On 6/5/17 12:32 PM, Dave Marion wrote: > >>> > The main entrance to the community for new contributors is through > pull requests. I have seen PR's approved in an inconsistent manner. My > intent was to make known the expectations for new contributions so that > newcomers don't get discouraged by the amount of feedback and/or changes > requested while providing some guidelines to make it more consistent. It > seems that there is not a desire to do this for various reasons. That's > fine by me and I'm willing to drop the discussion here. > >>> > > >>> > > >>> >> On June 5, 2017 at 12:14 PM "Marc P." <marc.par...@gmail.com> > wrote: > >>> >> > >>> >> 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 > mailto: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 > 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) > >>> >>> > >>> >>> > > >>> >> > >>> > > >>> > >
