Open Stack

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.

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'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).

> Another question I have is how to proceed with such changes. I've already
> implemented H306 (order of imports) and am being now puzzled with how to
> propose such change to Gerrit. This change naturally touches many files
> (163 so far) and as such is clearly not suited for review in one piece. The
> only solution I currently can think of is to split it in 4-5-6 patches
> without actually changing tox.ini, and after all of them are merged, issue
> a final patch that updates tox.ini and any files breaking the rule that
> were introduced in between. But there is still a question on how Jenkins
> works with verify and merge jobs. Can it happen that we end up with code in
> master that does not pass pep8 check? Or there will be a 'race condition'
> between my final patch and any other that breaks the style rules? I would
> really appreciate any thoughts and comments about this.

If you do proceed with the work, then I thing those reviewing will just
have to police the queue and ensure we don't merge patches which break the
style rules after you've fixed them.

Steve