geode-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Alexander Murmann <amurm...@pivotal.io>
Subject Re: [DISCUSS] require reviews before merging a PR
Date Fri, 31 May 2019 16:02:16 GMT
+1 to Jake's interpretation of the voting system not having been adjusted
yet for the new world that is GitHub. When I read the Apache voting
guidelines they make sense to me for bug decisions but not for a regular
PR.

The inertia coming with three votes on every PR is way more costly than the
alternative of an occasional rewind.

On Fri, May 31, 2019 at 8:52 AM Owen Nichols <onichols@pivotal.io> wrote:

> Apache requires 3 reviews for code changes. Docs and typos likely would not
> fall under that heading.
>
> On Fri, May 31, 2019 at 8:47 AM Dave Barnes <dbarnes@apache.org> wrote:
>
> > As a writer, I'm a big user of Lazy Consensus: If no one objects, I'm
> > merging my change. Requiring multiple reviews discourages minor
> > improvements. In the doc realm, I'm inclined to check in typo fixes and
> > grammar corrections without even bothering with the PR process, but I do
> it
> > for the community-ness of it. But requiring three reviews to correct a
> > spelling error is a big waste of the reviewers' time.
> >
> > On Thu, May 30, 2019 at 10:12 PM Jacob Barrett <jbarrett@pivotal.io>
> > wrote:
> >
> > >
> > >
> > > > On May 30, 2019, at 4:47 PM, Owen Nichols <onichols@pivotal.io>
> wrote:
> > > >
> > > > Some folks have found it really helpful to have the PR author
> schedule
> > a
> > > walk-through of the changes to give reviewers more context and explain
> > the
> > > thinking behind the changes.
> > >
> > > This can’t be policy unless the walkthrough is scheduled with the whole
> > > dev@geode community. You could say in your PR that a walkthrough will
> > > happen at a given time and location (online) so that interested parties
> > > could watch and ask questions. This strikes me as extremely onerous for
> > > most PRs. For large scale refactors, features, etc. maybe it makes
> sense,
> > > though for those a discussion thread should have happened on dev@geode
> > > first.
> > >
> > > -Jake
> > >
> > >
> >
>

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