mxnet-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Marco de Abreu <marco.g.ab...@googlemail.com>
Subject Re: [VOTE] When in Doubt, Wait 24 Hours Before Merging
Date Fri, 02 Feb 2018 23:03:31 GMT
I'm afraid that the approach starting the "timer" (I know, that's a bad
term) with the last commit does not really work out. There have been cases
in the past where a reviewer criticized something, but it was never
followed up by the contributor. As far as I understand your second
proposal, you would request the reviewer to check the pr every few days and
keep repeating themselves in order to remind the contributor until the
comment has been addressed?

I'd rather agree with Chris and his view that all comments have to be
addressed or (in case of no response from the reviewer after being
explicitly asked) a specific time has to pass in order to render a veto
invalid. With your proposal I'm afraid that, especially in big PRs, a veto
might get discarded without the reviewer actually being aware of it.

-Marco

Am 02.02.2018 2:07 nachm. schrieb "Sheng Zha" <szha.pvg@gmail.com>:

> Thanks for the comments. I'm considering the vote failed given the veto,
> and will draft another proposal.
>
> Isabel, thanks for bringing it up. I will update the grace period to 72
> hours in a new proposal.
>
> Chris, regarding the trigger, I think it should just be based on the latest
> commit on the PR (I will explain more below), and then I can add a term to
> recommend not to merge PR if another request for changes is received during
> grace period.
>
> Nan, indeed I think we committers should avoid forcing a change through,
> and reasonable amount of explanation should be given. The difficulty (and
> blessing) is the lack of authority for performing such approval. The
> intended situation is when one committer approves and the other
> disapproves.
>
> Chris and Marco, given that everything is publicly accessible, I don't
> think committer or requester should be obliged to additional informing
> duty. On github, one would be automatically subscribed to the PR for any
> further changes if one makes a comment or review. I agree that it's the
> shared responsibility of the change requester and the merging committer's
> to drive consensus. I also agree that it would be courteous for a committer
> to ping commenter to reach consensus, and it's the right thing for me to
> do. Still, I think it should be the commenter's responsibility to follow up
> on the prior, potentially outdated comment when needed. Veto is preserved
> in that a committer can close a PR at any time, and can revert changes or
> vetoing releases that contain the change.
>
> Lastly, I'd like to use the remainder of this thread to drive towards
> consensus on the revision needed for the proposal. In that, I welcome both
> further discussion on related points and counter proposals. Thank you.
>
> -sz
>
> On Fri, Feb 2, 2018 at 7:43 AM, Chris Olivier <cjolivier01@gmail.com>
> wrote:
>
> > -1 (binding) based upon:  It's not clear what triggers the timer or how a
> > veto is preserved.
> >
> > I'd like to argue that any comment on a PR by a committer should get some
> > sort of response, even if it is "bugger off", and the ability to veto
> must
> > be preserved (ie set review status as 'request changes')
> >
> > On Fri, Feb 2, 2018 at 6:18 AM, Isabel Drost-Fromm <isabel@apache.org>
> > wrote:
> >
> >>
> >>
> >> Am 2. Februar 2018 01:20:31 MEZ schrieb Sheng Zha <zhasheng@apache.org
> >:
> >> >Specifically, for merging PRs, if there are open review comments and
> >> >changes afterwards didn’t address the comments, we should have a
> >> >grace-period of 24 hours for commenters to respond to the changes.
> >>
> >> Typical waiting period for lazy consensus would be 72 hours IIRC so ppl
> >> in other timezones/ on holiday get a chance to check as well.
> >>
> >> Isabel
> >>
> >> --
> >> Diese Nachricht wurde von meinem Android-Gerät mit K-9 Mail gesendet.
> >>
> >
> >
>

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