On Tue, Aug 6, 2013 at 1:44 PM, Morgan Fainberg <m...@metacloud.com> wrote:
> While I'm torn on this as a developer, it comes down to an ease of > understanding the code. In all cases, it is easier to understand where > something comes from if you only import modules. Enforcing the import of > modules tends to also ensure namespace conflicts don't occur as often. > When it comes to review, I am going to agree with Sean here, it is a boon > on large changes. I am against lessening/removing H302; but I understand > why people desire it eased up. I agree with Robert, that this can be annoying, but as Sean so eloquently said one of the main goals of hacking is to reduce the burden on reviews and to prevent two reviewers from telling the person opposite things. The constrained resource in development today is reviewers not developers, so I am inclined to go with what makes reviewing easier. In some ways an insane and annoying style requirement, is better then none as long as it makes code more uniform and makes reviewing easier. So I too am a soft -1 on dropping this, especially because individual projects can ignore this per line or per project. > > Cheers, > Morgan Fainberg > > IRC: morganfainberg > > > On Tue, Aug 6, 2013 at 1:18 PM, Christopher Armstrong < > chris.armstr...@rackspace.com> wrote: > >> On Tue, Aug 6, 2013 at 6:32 AM, Sean Dague <s...@dague.net> wrote: >> >>> >>> The reason we go hard and fast on certain rules is to reduce review time >>> by people. If something is up for debate we get bikeshedding in reviews >>> where one reviewer tells someone to do it one way, 2 days later they update >>> their review, another reviewer comes in and tells them to do it the >>> otherway. (This is not theoretical, it happens quite often, if you do a lot >>> of reviews you see it all the time.) It also ends up being something >>> reviewers can stop caring about, because the machine will pick it up. >>> Giving them the ability to focus on higher order issues, and still keeping >>> the code from natural entropy. >>> >>> MUST == computer can do it, less work for core review time (which is >>> realistically one of our most constrained resources in OpenStack) >>> MAY == humans have to make a judgement call, which means more work for >>> our already constrained review teams >>> >>> I've found H302 to really be useful on reviewing large chunks of code >>> I've not been in much before. And get seriously annoyed being in projects >>> that don't have it enforced yet (tempest is guilty of that). Being able to >>> quickly know what namespace things are out of saves time. >>> >> >> >> I think it's really unfortunate that people will block patches based on >> stylistic concerns. The answer, IMO, is to codify in policy that stylistic >> issues *cannot* block a patch from landing. >> >> I recommend having humility in our reviews. Instead of >> >> "This bike shed needs to be painted red. -1" >> >> One should say >> >> "I prefer red for the color of bike sheds. You can do that if you want, >> but go ahead and merge anyway if you don't want to. +0" >> >> and don't mark a review as -1 if it *only* has bikeshedding in it. I >> would love to see a culture of reviewing that emphasizes functional >> correctness, politeness, and mutual education. >> >> And given the rationale from Robert Collins, I agree that the >> module-import thing should be one of the flakes that allows exceptions. >> > It already is, all imports support the #noqa comment which tells flake8 to ignore the line. > >> -- >> IRC: radix >> Christopher Armstrong >> Rackspace >> >> _______________________________________________ >> OpenStack-dev mailing list >> OpenStack-dev@lists.openstack.org >> http://lists.openstack.org/cgi-bin/mailman/listinfo/openstack-dev >> >> > > _______________________________________________ > OpenStack-dev mailing list > OpenStack-dev@lists.openstack.org > http://lists.openstack.org/cgi-bin/mailman/listinfo/openstack-dev > >
_______________________________________________ OpenStack-dev mailing list OpenStack-dev@lists.openstack.org http://lists.openstack.org/cgi-bin/mailman/listinfo/openstack-dev
