Open Stack

On Tue, Dec 16, 2014 at 3:22 PM, Ben Nemec <openstack at nemebean.com> 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.
>

++, 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
> >>> 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
> >>
> >
> >
>
>
> _______________________________________________
> OpenStack-dev mailing list
> OpenStack-dev at lists.openstack.org
> http://lists.openstack.org/cgi-bin/mailman/listinfo/openstack-dev
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.openstack.org/pipermail/openstack-dev/attachments/20141216/0dccca4d/attachment.html>