[openstack-dev] [hacking] proposed rules drop for 1.0

Sean Dague sean at dague.net
Wed Dec 17 11:14:51 UTC 2014

On 12/16/2014 06:22 PM, Ben Nemec wrote:
> 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).

The issue I originally had was in nova.tests.fixtures, where it resolved
fixtures as a relative import instead of an absolute one, and exploded.
It's not reproducing now though.

> 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.

H304 needs is_import_exception.

is_module and is_import_exception means we have to import all the code,
which means the depends for pep8 is *all* of requirements.txt, all of
test-requirements.txt all of any optional (not listed in those
requirements). If the content isn't in the venv, the check passes. So
adding / removing an optional requirement can change the flake8 test

Evaluating the code is something that we should avoid.


Sean Dague

More information about the OpenStack-dev mailing list