accumulo-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Christopher <>
Subject Re: [DISCUSS] Pull Request Guidelines
Date Mon, 05 Jun 2017 15:59:50 GMT
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. <> 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 <> 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)
> >

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