Open Stack

On 2014-01-22 22:13, Zane Bitter wrote:
> 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.

It's rather unfortunate that the convention for "discard this tuple 
element" is _, which conflicts with translation code if you happen to 
have any translated strings that show up after the discarded _ is 
assigned.  The only solution I see is to use a different convention for 
hacking to indicate that a variable is throwaway.  Unfortunately this 
isn't one of our hacking checks so I think we'd have to reimplement the 
flake8 check to recognize the new convention. :-/

> 
>> 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.
> 
> _______________________________________________
> OpenStack-dev mailing list
> OpenStack-dev at lists.openstack.org
> http://lists.openstack.org/cgi-bin/mailman/listinfo/openstack-dev