Great. Thanks for the clarifications everyone. I think we are good then :) - Carin
On Tue, Nov 20, 2018 at 10:43 AM kellen sunderland < [email protected]> wrote: > 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 <[email protected]> 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 <[email protected] > >: > > > > > 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 <[email protected]> > > 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 > > > > > >
