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 16:46:12 GMT
I think consistency is a good thing to strive for, but I think
inconsistency is unavoidable, especially in an all-volunteer community and
where each contribution is unique. I like Mike Walch's suggestion about how
to clearly phrase things. I think recommended phrasing might make for good
guidelines on how to perform a review.

On Mon, Jun 5, 2017 at 12:41 PM Mike Walch <mwalch@apache.org> wrote:

> I agree with Christopher about avoiding strict guidelines for reviews but
> having some loose guidelines/advice for reviewers.
>
> One thing that I have found helpful is when reviewers communicate how
> strongly they feel about a change:
>
> - If the change is optional:       Could add a unit test for this class
> - If the change is suggested:   A unit test should be added for this class
> - If the change is required:       I am -1 on this PR until a unit test is
> added for this class.
>
> As a reviewer, I really like using "Could" before my PR comments.  It lets
> me suggest whatever change I want without forcing the contributor to make
> these changes if they disagree.
>
> On Mon, Jun 5, 2017 at 12:00 PM Christopher <ctubbsii@apache.org> wrote:
>
> > I'm not entirely sure what such specific guidelines hope to achieve. I'm
> in
> > favor of having some "things to look for" guidelines, but I don't think
> > strict guidelines are going to work, because it elevates rules over the
> > human element. I don't think we can be too prescriptive about what
> things a
> > contributor *should* do, or what a reviewer *should* check for, because
> > everybody has unique volunteer time constraints, expertise, and passions.
> >
> > I'm also concerned about long-term maintenance of any such documentation,
> > which seems likely to quickly get out of date as the community itself
> > evolves, if we are too specific. Any such guidelines should be
> maintainable
> > by being succinct, generic, and flexible. I'm thinking something like:
> >
> > 1. Follow the ASF Code of Conduct [link]
> > 2. Ensure build passes (captures most checks; can rely on Travis CI)
> > [suggested build command-line]
> > 3. Check for style and formatting failures, or uncommitted changes from
> the
> > build tooling after a build
> > 5. Consider semver rules [link], especially when contributing to
> > maintenance branches
> > 6. Add tests when appropriate
> > 7. Be willing to discuss performance impact, API design, style choices
> >
> > The key thing about contributing and reviewing is being willing to engage
> > in polite discussion about any part of the contribution. Too much
> > specificity will result in docs which don't apply to majority of
> > circumstances, or docs which become out-of-date quickly, or docs which
> > nobody reads because they are too complicated or long. The upfront costs
> of
> > trying to make sure you comply with all possible guidelines may also be a
> > deterrence from contributing. It's much easier to be willing to receive
> > friendly feedback than it is to try to make sure you take care of any
> > possible thing somebody might result in a change suggestion.
> >
> > On Mon, Jun 5, 2017 at 11:25 AM Marc P. <marc.parisi@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 <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