accumulo-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Dave Marion <dlmar...@comcast.net>
Subject Re: [DISCUSS] Pull Request Guidelines
Date Mon, 05 Jun 2017 15:47:41 GMT
That's the intent. If we have some guidelines, and they are read first by a new contributor,
then they will know that their is a formatter that they should be using.

> On June 5, 2017 at 11:35 AM Mike Drob <mdrob@apache.org> wrote:
>
>
> > 1. Adherence to code formatting rules (link to formatting rules)
>
> Can we let checkstyle handle this instead of humans worrying about it?
>
> On Mon, Jun 5, 2017 at 10: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