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" <[email protected]>: > 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 <[email protected]> > 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 <[email protected]> > > wrote: > > > >> > >> > >> Am 2. Februar 2018 01:20:31 MEZ schrieb Sheng Zha <[email protected] > >: > >> >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. > >> > > > > >
