Hey Carin, I don't think there's any issues merging this PR. The veto'd aspect was around _requiring_ modern loop usage, and failing the build if clang tidy detected modern loops could be used but weren't. The original PR included a check for this and would fail any builds not using modern loops. Several people didn't like this aspect so I updated the PR and removed that overly-strict check. The current PR doesn't have anything it in that's been vetod. We're continuing to warn if clang-tidy detects a loop could be modernized, but are not causing an error (which was actually the behaviour before this PR was merged).
On Tue, Nov 20, 2018 at 7:29 AM Anton Chernov <mecher...@gmail.com> wrote: > Hi Carin, > > The discussion [1] was about whether to enable automatic checks on using > old behaviour in new PR's. Kellens PR [2] was about modernizing the actual > code itself and was not up for voting, thus could not receive any technical > veto votes. > > Per the discussion (as I have understood it), we won't get veto votes if we > would enable the check on CI, if it would be treated as a warning. > > Thank you for merging the PR in the first place. I see no reason for > reverting it. > > Best > Anton > > [1] > > https://lists.apache.org/thread.html/b47f285a80bef47c5ead6c361614e338a0661f6c0c76196c1e3719c5@%3Cdev.mxnet.apache.org%3E > [2] https://github.com/apache/incubator-mxnet/pull/12356 > > > вт, 20 нояб. 2018 г. в 15:24, Pedro Larroy <pedro.larroy.li...@gmail.com>: > > > Hi all > > > > I think we have to make the clear separation between the thread votes > > on "uniformly adopting C++11 range loops in the MXNet project" and a > > PR which refactored code to be more legible and with improved variable > > names. > > Merging that PR doesn't imply that we have to uniformly adopt the > > previous proposal. The PR was reviewed and approved by several > > people. I would keep the two topics separate, merging this PR doesn't > > prescribe any particular idiom for future commits or reviews. > > > > Pedro. > > > > On Tue, Nov 20, 2018 at 2:58 PM Carin Meier <carinme...@gmail.com> > wrote: > > > > > > My intent was to be helpful, but I think I may have merged this PR > > > yesterday too soon thinking it was approved and ready to merge > > > https://github.com/apache/incubator-mxnet/pull/12356 > > > > > > I didn't see the connected dev discussion > > > > > > https://lists.apache.org/thread.html/b47f285a80bef47c5ead6c361614e338a0661f6c0c76196c1e3719c5@%3Cdev.mxnet.apache.org%3E > > > where there were -1 votes, which I believe are vetos? > > > > > > So the question is confirm: should PR should be reverted? > > > > > > Sorry for any confusion, > > > Carin > > >
