<div dir="ltr"><br><div class="gmail_extra"><br><div class="gmail_quote">On Tue, Dec 16, 2014 at 3:22 PM, Ben Nemec <span dir="ltr"><<a href="mailto:openstack@nemebean.com" target="_blank">openstack@nemebean.com</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex">Some thoughts inline.  I'll go ahead and push a change to remove the<br>
things everyone seems to agree on.<br>
<div><div class="h5"><br>
On 12/09/2014 09:05 AM, Sean Dague wrote:<br>
> On 12/09/2014 09:11 AM, Doug Hellmann wrote:<br>
>><br>
>> On Dec 9, 2014, at 6:39 AM, Sean Dague <<a href="mailto:sean@dague.net">sean@dague.net</a>> wrote:<br>
>><br>
>>> I'd like to propose that for hacking 1.0 we drop 2 groups of rules entirely.<br>
>>><br>
>>> 1 - the entire H8* group. This doesn't function on python code, it<br>
>>> functions on git commit message, which makes it tough to run locally. It<br>
>>> also would be a reason to prevent us from not rerunning tests on commit<br>
>>> message changes (something we could do after the next gerrit update).<br>
>>><br>
>>> 2 - the entire H3* group - because of this -<br>
>>> <a href="https://review.openstack.org/#/c/140168/2/nova/tests/fixtures.py,cm" target="_blank">https://review.openstack.org/#/c/140168/2/nova/tests/fixtures.py,cm</a><br>
>>><br>
>>> A look at the H3* code shows that it's terribly complicated, and is<br>
>>> often full of bugs (a few bit us last week). I'd rather just delete it<br>
>>> and move on.<br>
>><br>
>> I don’t have the hacking rules memorized. Could you describe them briefly?<br>
><br>
> Sure, the H8* group is git commit messages. It's checking for line<br>
> length in the commit message.<br>
><br>
> - [H802] First, provide a brief summary of 50 characters or less.  Summaries<br>
>   of greater then 72 characters will be rejected by the gate.<br>
><br>
> - [H801] The first line of the commit message should provide an accurate<br>
>   description of the change, not just a reference to a bug or<br>
>   blueprint.<br>
><br>
><br>
> H802 is mechanically enforced (though not the 50 characters part, so the<br>
> code isn't the same as the rule).<br>
><br>
> H801 is enforced by a regex that looks to see if the first line is a<br>
> launchpad bug and fails on it. You can't mechanically enforce that<br>
> english provides an accurate description.<br>
<br>
</div></div>+1.  It would be nice to provide automatic notification to people if<br>
they submit something with an absurdly long commit message, but I agree<br>
that hacking isn't the place to do that.<br>
<span class=""><br>
><br>
><br>
> H3* are all the module import rules:<br>
><br>
> Imports<br>
> -------<br>
> - [H302] Do not import objects, only modules (*)<br>
> - [H301] Do not import more than one module per line (*)<br>
> - [H303] Do not use wildcard ``*`` import (*)<br>
> - [H304] Do not make relative imports<br>
> - Order your imports by the full module path<br>
> - [H305 H306 H307] Organize your imports according to the `Import order<br>
>   template`_ and `Real-world Import Order Examples`_ below.<br>
><br>
> I think these remain reasonable guidelines, but H302 is exceptionally<br>
> tricky to get right, and we keep not getting it right.<br>
><br>
> H305-307 are actually impossible to get right. Things come in and out of<br>
> stdlib in python all the time.<br>
<br>
</span>tdlr; I'd like to remove H302, H305 and, H307 and leave the rest.<br>
Reasons below.<br>
<br>
+1 to H305 and H307.  I'm going to have to admit defeat and accept that<br>
I can't make them work in a sane fashion.<br></blockquote><div><br></div><div>++, these have been nothing but trouble.</div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex">
<br>
H306 is different though - that one is only checking alphabetical order<br>
and only works on the text of the import so it doesn't have the issues<br>
around having modules installed or mis-categorizing.  AFAIK it has never<br>
actually caused any problems either (the H306 failure in<br>
<a href="https://review.openstack.org/#/c/140168/2/nova/tests/unit/test_fixtures.py" target="_blank">https://review.openstack.org/#/c/140168/2/nova/tests/unit/test_fixtures.py</a><br>
is correct - nova.tests.fixtures should come before<br>
nova.tests.unit.conf_fixture).<br></blockquote><div><br></div><div>Agreed H306 is mechanically enforceable and is  there in part to reduce the risk of merge conflicts in the imports section</div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex">
<br>
As far as 301-304, only 302 actually depends on the is_module stuff.<br>
The others are all text-based too so I think we should leave them.  H302<br>
I'm kind of indifferent on - we hit an edge case with the olso namespace<br>
thing which is now fixed, but if removing that allows us to not install<br>
requirements.txt to run pep8 I think I'm onboard with removing it too.<br></blockquote><div><br></div><div>As for H302, it comes from <a href="https://google-styleguide.googlecode.com/svn/trunk/pyguide.html?showone=Imports#Imports">https://google-styleguide.googlecode.com/svn/trunk/pyguide.html?showone=Imports#Imports</a></div><div><br></div><div>We still don't have this one working right, running flake8 outside a venv  is still causing oslo packaging related issues for me</div><div><br></div><div><div>./nova/i18n.py:21:1: H302  import only modules.'from oslo import i18n' does not import a module</div></div><div><br></div><div>So +1 to just removing it.</div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex">
<div class=""><div class="h5"><br>
><br>
><br>
> I think it's time to just decide to be reasonable Humans and that these<br>
> are guidelines.<br>
><br>
> The H3* set of rules is also why you have to install *all* of<br>
> requirements.txt and test-requirements.txt in your pep8 tox target,<br>
> because H302 actually inspects the sys.modules to attempt to figure out<br>
> if things are correct.<br>
><br>
>       -Sean<br>
><br>
>><br>
>> Doug<br>
>> - [H802] First, provide a brief summary of 50 characters or less.  Summaries<br>
>   of greater then 72 characters will be rejected by the gate.<br>
><br>
> - [H801] The first line of the commit message should provide an accurate<br>
>   description of the change, not just a reference to a bug or<br>
>   blueprint.<br>
><br>
>><br>
>>><br>
>>>     -Sean<br>
>>><br>
>>> --<br>
>>> Sean Dague<br>
>>> <a href="http://dague.net" target="_blank">http://dague.net</a><br>
>>><br>
>>> _______________________________________________<br>
>>> OpenStack-dev mailing list<br>
>>> <a href="mailto:OpenStack-dev@lists.openstack.org">OpenStack-dev@lists.openstack.org</a><br>
>>> <a href="http://lists.openstack.org/cgi-bin/mailman/listinfo/openstack-dev" target="_blank">http://lists.openstack.org/cgi-bin/mailman/listinfo/openstack-dev</a><br>
>><br>
>><br>
>> _______________________________________________<br>
>> OpenStack-dev mailing list<br>
>> <a href="mailto:OpenStack-dev@lists.openstack.org">OpenStack-dev@lists.openstack.org</a><br>
>> <a href="http://lists.openstack.org/cgi-bin/mailman/listinfo/openstack-dev" target="_blank">http://lists.openstack.org/cgi-bin/mailman/listinfo/openstack-dev</a><br>
>><br>
><br>
><br>
<br>
<br>
_______________________________________________<br>
OpenStack-dev mailing list<br>
<a href="mailto:OpenStack-dev@lists.openstack.org">OpenStack-dev@lists.openstack.org</a><br>
<a href="http://lists.openstack.org/cgi-bin/mailman/listinfo/openstack-dev" target="_blank">http://lists.openstack.org/cgi-bin/mailman/listinfo/openstack-dev</a><br>
</div></div></blockquote></div><br></div></div>