Open Stack

On Thu, Jan 23, 2014 at 07:54:34AM +1000, Angus Salkeld wrote:
> On 22/01/14 12:21 +0000, Steven Hardy wrote:
> >On Wed, Jan 22, 2014 at 01:23:05PM +0200, Pavlo Shchelokovskyy wrote:
> >>Hi all,
> >>
> >>we have an approved blueprint that concerns reducing number of ignored PEP8
> >>and openstack/hacking style checks for heat (
> >>https://blueprints.launchpad.net/heat/+spec/reduce-flake8-ignored-rules).
> >>I've been already warned that enabling some of these rules will be quite
> >>controversial, and personally I do not like some of these rules myself
> >>either. In order to understand what is the opinion of the community, I
> >>would like to ask you to leave a comment on the blueprint page about what
> >>do you think about enabling these checks.
> >>
> >>The style rules being currently ignored are:
> >>F841 local variable 'json_template' is assigned to but never used
> >
> >This was fixed an enabled in https://review.openstack.org/#/c/62827/
> >
> >>H201 no 'except:' at least use 'except Exception:' (this actually checks
> >>for bare 'except:' lines, so 'except BaseException:' will pass too)
> >
> >This sounds reasonable, we made an effort to purge naked excepts a while
> >back so hopefully it shouldn't be too difficult to enable.
> >
> >However there are a couple of remaining instances (in resource.py and
> >scheduler.py in particular), so we need to evaluate if these are
> >justifiable or need to be reworked.
> >
> >>H302 do not import objects, only modules (this I don't like myself as it
> >>can clutter the code beyond reasonable limit)
> >>H306 imports not in alphabetical order
> >>H404 multi line docstring should start with a summary
> >
> >Personally I don't care much about any of these, in particular the import
> >ones seem to me unncessesarily inconvenient so I'd prefer to leave these
> >disabled.
> 
> :( I like the import checks

Ok, well whatever I guess.

My argument against the alpha-sort of imports, is that it basically
randomizes the structure from the perspective of how most people (or maybe
it's just me..) think about the imports.

I always think of imports in terms of where they come from, ie in order of
preference:

1. Import a core python module
2. Import some module available via pypi
3. Import some module from oslo sync'd to $project/openstack/common
4. Import something local to the project

This results in something like:

import functools
import json

from oslo.config import cfg
import webob

from heat.openstack.common import timeutils
from heat.openstack.common import log as logging
from heat.openstack.common import threadgroup
from heat.openstack.common.gettextutils import _
from heat.openstack.common.rpc import service
from heat.openstack.common.rpc import common as rpc_common
from heat.openstack.common import excutils
from heat.openstack.common import uuidutils

from heat.common import context
from heat.common import exception
from heat.common import identifier
from heat.common import heat_keystoneclient as hkc
from heat.db import api as db_api
from heat.rpc import api as rpc_api
from heat.engine import api
from heat.engine import attributes
from heat.engine import clients
from heat.engine.event import Event
from heat.engine import environment
from heat.engine import parameters
from heat.engine import parser
from heat.engine import properties
from heat.engine import resource
from heat.engine import resources
from heat.engine import stack_lock
from heat.engine import template as tpl
from heat.engine import watchrule

Or, if you go with the lets-alpha-sort-everything approach:

from heat.common import context
from heat.common import exception
from heat.common import heat_keystoneclient as hkc
from heat.common import identifier
from heat.db import api as db_api
from heat.engine.event import Event
from heat.engine import api
from heat.engine import attributes
from heat.engine import clients
from heat.engine import environment
from heat.engine import parameters
from heat.engine import parser
from heat.engine import properties
from heat.engine import resource
from heat.engine import resources
from heat.engine import stack_lock
from heat.engine import template as tpl
from heat.engine import watchrule
from heat.openstack.common.gettextutils import _
from heat.openstack.common import excutils
from heat.openstack.common import log as logging
from heat.openstack.common import threadgroup
from heat.openstack.common import timeutils
from heat.openstack.common import uuidutils
from heat.openstack.common.rpc import common as rpc_common
from heat.openstack.common.rpc import service
from heat.rpc import api as rpc_api
from oslo.config import cfg
import functools
import json
import webob

Which to me is not only much less readable, but also I have to expend
valuable brainpower (hey, it's a limited resource ok! :) mentally alpha
sorting every new import I add to every file.

So that's my argument against the alpha-sort OCD-ness, FWIW.

> >H404 is probably a stronger argument, as it would help improve the
> >quality of our auto-generated docs, but again I see it as of marginal
> >value considering the (probably large) effort involved.
> 
> I tried to fix this a while ago and it's a big patch. I'd suggest
> you break it into a bunch of patches.
> 
> >
> >I'd rather see that effort used to provide a better, more automated way to
> >keep our API documentation updated (since that's the documentation users
> >really need, combined with the existing template/resource documentation).
> 
> People do what they feel comfortable with..

Yes *of course* people can spend their time doing whatever they wish, but I
felt obligated to point out that these changes are mostly completely
invisible to our users and the H404 change will involve considerable effort
fixing something which, probably, hardly anyone will even notice.

Steve