Open Stack

On 22/01/14 06:23, 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

It would be interesting to do an audit and find out how many of these 
are actual dead code and how many are e.g. legitimate discarding of 
returned tuple components. If it's almost exclusively the former, fine. 
However, for the latter there's no obvious substitute that doesn't make 
the code considerably worse, so if the frequency of that is at all 
significant then this is probably counter-productive.

> H201 no 'except:' at least use 'except Exception:' (this actually checks
> for bare 'except:' lines, so 'except BaseException:' will pass too)

I don't think this check is sophisticated enough. A bare "except:" is 
legit provided that it always re-raises the exception. I believe we 
already got rid of all the non-legitimate uses, and the code review 
process has proven adequate to keep them out (somebody always asks about 
it, even if it is legit).

(We have bigger problems with blanket "except Exception:" catching, but 
those are even harder again to detect.)

> H302 do not import objects, only modules (this I don't like myself as it
> can clutter the code beyond reasonable limit)

I would actually like to see this one happen. We're still adding new 
violations of this, often without any particularly strong reason, and 
code review doesn't seem to be sufficient to keep them out.

> H306 imports not in alphabetical order

This would be nice too. It's hard to know where to put stuff when it's 
all jumbled up already.

> H404 multi line docstring should start with a summary

This is a massive task for not a lot of benefit IMHO. I think at one 
point there was an attempt to do this automatically, but at the end of 
the day sticking a random full stop + newline in the middle of the first 
sentence of every docstring does not really do anything to make the 
world a better place.

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

No, there's no race condition. Every patch has to pass the gate after 
being merged before it is delivered.

cheers,
Zane.