Open Stack

Some thoughts inline.  I'll go ahead and push a change to remove the
things everyone seems to agree on.

On 12/09/2014 09:05 AM, Sean Dague wrote:
> On 12/09/2014 09:11 AM, Doug Hellmann wrote:
>>
>> On Dec 9, 2014, at 6:39 AM, Sean Dague <sean at dague.net> wrote:
>>
>>> I'd like to propose that for hacking 1.0 we drop 2 groups of rules entirely.
>>>
>>> 1 - the entire H8* group. This doesn't function on python code, it
>>> functions on git commit message, which makes it tough to run locally. It
>>> also would be a reason to prevent us from not rerunning tests on commit
>>> message changes (something we could do after the next gerrit update).
>>>
>>> 2 - the entire H3* group - because of this -
>>> https://review.openstack.org/#/c/140168/2/nova/tests/fixtures.py,cm
>>>
>>> A look at the H3* code shows that it's terribly complicated, and is
>>> often full of bugs (a few bit us last week). I'd rather just delete it
>>> and move on.
>>
>> I don’t have the hacking rules memorized. Could you describe them briefly?
> 
> Sure, the H8* group is git commit messages. It's checking for line
> length in the commit message.
> 
> - [H802] First, provide a brief summary of 50 characters or less.  Summaries
>   of greater then 72 characters will be rejected by the gate.
> 
> - [H801] The first line of the commit message should provide an accurate
>   description of the change, not just a reference to a bug or
>   blueprint.
> 
> 
> H802 is mechanically enforced (though not the 50 characters part, so the
> code isn't the same as the rule).
> 
> H801 is enforced by a regex that looks to see if the first line is a
> launchpad bug and fails on it. You can't mechanically enforce that
> english provides an accurate description.

+1.  It would be nice to provide automatic notification to people if
they submit something with an absurdly long commit message, but I agree
that hacking isn't the place to do that.

> 
> 
> H3* are all the module import rules:
> 
> Imports
> -------
> - [H302] Do not import objects, only modules (*)
> - [H301] Do not import more than one module per line (*)
> - [H303] Do not use wildcard ``*`` import (*)
> - [H304] Do not make relative imports
> - Order your imports by the full module path
> - [H305 H306 H307] Organize your imports according to the `Import order
>   template`_ and `Real-world Import Order Examples`_ below.
> 
> I think these remain reasonable guidelines, but H302 is exceptionally
> tricky to get right, and we keep not getting it right.
> 
> H305-307 are actually impossible to get right. Things come in and out of
> stdlib in python all the time.

tdlr; I'd like to remove H302, H305 and, H307 and leave the rest.
Reasons below.

+1 to H305 and H307.  I'm going to have to admit defeat and accept that
I can't make them work in a sane fashion.

H306 is different though - that one is only checking alphabetical order
and only works on the text of the import so it doesn't have the issues
around having modules installed or mis-categorizing.  AFAIK it has never
actually caused any problems either (the H306 failure in
https://review.openstack.org/#/c/140168/2/nova/tests/unit/test_fixtures.py
is correct - nova.tests.fixtures should come before
nova.tests.unit.conf_fixture).

As far as 301-304, only 302 actually depends on the is_module stuff.
The others are all text-based too so I think we should leave them.  H302
I'm kind of indifferent on - we hit an edge case with the olso namespace
thing which is now fixed, but if removing that allows us to not install
requirements.txt to run pep8 I think I'm onboard with removing it too.

> 
> 
> I think it's time to just decide to be reasonable Humans and that these
> are guidelines.
> 
> The H3* set of rules is also why you have to install *all* of
> requirements.txt and test-requirements.txt in your pep8 tox target,
> because H302 actually inspects the sys.modules to attempt to figure out
> if things are correct.
> 
> 	-Sean
> 
>>
>> Doug
>> - [H802] First, provide a brief summary of 50 characters or less.  Summaries
>   of greater then 72 characters will be rejected by the gate.
> 
> - [H801] The first line of the commit message should provide an accurate
>   description of the change, not just a reference to a bug or
>   blueprint.
> 
>>
>>>
>>> 	-Sean
>>>
>>> -- 
>>> Sean Dague
>>> http://dague.net
>>>
>>> _______________________________________________
>>> OpenStack-dev mailing list
>>> OpenStack-dev at lists.openstack.org
>>> http://lists.openstack.org/cgi-bin/mailman/listinfo/openstack-dev
>>
>>
>> _______________________________________________
>> OpenStack-dev mailing list
>> OpenStack-dev at lists.openstack.org
>> http://lists.openstack.org/cgi-bin/mailman/listinfo/openstack-dev
>>
> 
>