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 <s...@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
results.
Evaluating the code is something that we should avoid.
-Sean
--
Sean Dague
http://dague.net
_______________________________________________
OpenStack-dev mailing list
OpenStack-dev@lists.openstack.org
http://lists.openstack.org/cgi-bin/mailman/listinfo/openstack-dev
