On Tue, Dec 16, 2014 at 3:22 PM, Ben Nemec <[email protected]> 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 <[email protected]> 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. > ++, these have been nothing but trouble. > > 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). > Agreed H306 is mechanically enforceable and is there in part to reduce the risk of merge conflicts in the imports section > > 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. > As for H302, it comes from https://google-styleguide.googlecode.com/svn/trunk/pyguide.html?showone=Imports#Imports We still don't have this one working right, running flake8 outside a venv is still causing oslo packaging related issues for me ./nova/i18n.py:21:1: H302 import only modules.'from oslo import i18n' does not import a module So +1 to just removing it. > > > > > > > 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 > >>> [email protected] > >>> http://lists.openstack.org/cgi-bin/mailman/listinfo/openstack-dev > >> > >> > >> _______________________________________________ > >> OpenStack-dev mailing list > >> [email protected] > >> http://lists.openstack.org/cgi-bin/mailman/listinfo/openstack-dev > >> > > > > > > > _______________________________________________ > OpenStack-dev mailing list > [email protected] > http://lists.openstack.org/cgi-bin/mailman/listinfo/openstack-dev >
_______________________________________________ OpenStack-dev mailing list [email protected] http://lists.openstack.org/cgi-bin/mailman/listinfo/openstack-dev
