Open Stack

On 2014-01-23 04:16, Steven Hardy wrote:
> 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.

FWIW, this isn't what the hacking guidelines suggest, but the test to 
enforce the rest of the import ordering guidelines isn't quite in yet 
(it's this >< close though).  Hacking actually says something similar to 
what you suggest, except that it requires alpha sorting the individual 
groups and it combines the oslo imports and the project ones.  So you 
end up with three groups:

stdlib imports

third party lib imports

project imports (including oslo)

All of which are alpha sorted individually.  Not exactly what you 
suggested above, but I think it's in the same spirit.

-Ben