accumulo-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Christopher <ctubb...@apache.org>
Subject Re: [DISCUSS] Pull Request Guidelines
Date Mon, 05 Jun 2017 19:17:12 GMT
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 <keith@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 <michaelpmiller@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 <dlmarion@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.elser@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.parisi@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 <keith@deenlo.com
> mailto:keith@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 <dlmarion@comcast.net
> mailto:dlmarion@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)
> >>> >>>
> >>> >>> >
> >>> >>
> >>> >
> >>> >
>

Mime
  • Unnamed multipart/alternative (inline, None, 0 bytes)
View raw message