Open Stack

On 08/06/2013 07:32 AM, Sean Dague wrote:
> On 08/05/2013 10:38 PM, Monty Taylor wrote:
>>
>>
>> On 08/05/2013 11:26 PM, Robert Collins wrote:
>>> I wanted to get a temperature reading from everyone on this style
>>> guideline.
>>>
>>> My view on it is that it's a useful heuristic but shouldn't be a
>>> golden rule applied everywhere. Things like matches are designed to be
>>> used as a dsl:
>>>      self.assertThat(foo, Or(Equals("1"), Equals("2")))
>>>
>>> rather than what H302 enforces:
>>>      self.assertThat(foo, matchers.Or(matchers.Equals("1"),
>>> matchers.Equals("2")))
>>>
>>> Further, conflicting module names become harder to manage, when one
>>> could import just the thing.
>>>
>>> Some arguments for requiring imports of modules:
>>>   - makes the source of symbols obvious
>>>     - Actually, it has no impact on that as the import is still present
>>> and clear in the file. import * would obfuscate things, but I'm not
>>> arguing for that.
>>>     - and package/module names can (and are!) still ambiguous. Like
>>> 'test.' - whats that? -> consult the imports.
>>>   - makes mocking more reliable
>>>     - This is arguably the case, but it's a mirage: it isn't a complete
>>> solution because modules still need to be mocked at every place they
>>> are dereferenced : only import modules helps to the extent that one
>>> never mocks modules. Either way this failure mode of mocking is
>>> usually very obvious IME : but keeping the rule as a recommendation,
>>> *particularly* when crossing layers to static resources is a good
>>> idea.
>>>   - It's in the Google Python style guide
>>> (http://google-styleguide.googlecode.com/svn/trunk/pyguide.html?showone=Imports#Imports)
>>>
>>>     - shrug :)
>>>
>>> What I'd like us to do is weaken it from a MUST to a MAY, unless noone
>>> cares about it at all, in which case lets just turn it off entirely.
>>
>> Enforcing it is hard. The code that does it has to import and then make
>> guesses on failures.
>>
>> Also - I agree with Robert on this. I _like_ writing my code to not
>> import bazillions of things... but I think the hard and fast rule makes
>> things crappy at times.
>
> 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.
>
> Honestly, after spending the year with the constraint in OpenStack, I'm
> never going to import modules directly in my personal projects, as I
> think the benefits of the explicitness have shown themselves pretty well.
>
> So I'm a soft -1 on dropping it from hacking.

+1 from me (or another -1 for dropping it from hacking). Sean's points 
above about reviewer time and reducing the back and forth bickering on 
bikeshedding issues is why the rule enforcement is a good idea.

Plus, if someone really wanted to set Or and LessThan into the modules 
local scope, you could just do:

from testtools import matchers
...

Or = matchers.Or
LessThan = matchers.LessThan
...

That way you get both clarity/explicitness as well as convenient 
shortened names.

-jay