I have used Hadoop's documentation on this subject for submitting patches. I'm not suggesting that we go to this level of detail, but as a new contributor I know how to set up my IDE, what commands to run to create my patch, and I know the items that are going to be checked at the start.
[1] https://wiki.apache.org/hadoop/HowToContribute [2] https://wiki.apache.org/hadoop/CodeReviewChecklist > > On June 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) > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > >
