Re: Should PR-860 (Use modernized range loops where possible) be reverted?
Great. Thanks for the clarifications everyone. I think we are good then :) - Carin On Tue, Nov 20, 2018 at 10:43 AM kellen sunderland < kellen.sunderl...@gmail.com> 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 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 >: > > > > > 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 > > 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 > > > > > >
Re: Should PR-860 (Use modernized range loops where possible) be reverted?
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 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 : > > > 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 > 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 > > >
Re: Should PR-860 (Use modernized range loops where possible) be reverted?
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 : > 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 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 >
Re: Should PR-860 (Use modernized range loops where possible) be reverted?
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 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
Should PR-860 (Use modernized range loops where possible) be reverted?
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
