Open Stack

On 06/24/2014 12:34 AM, Angus Salkeld wrote:
> On 24/06/14 12:59, Joe Gordon wrote:
>> Hi All,
> 
>> After Friday's thread on removing several hacking rules. H402 and H803 are lines 
>> up to removed in the next few days, while Ben has volunteered to work on H305. 
>> In addition to helping clarify if we can remove a few specific rules, the thread 
>> touched on the bigger issue of how do make sure the rules in hacking reflect the 
>> communities lazy consensus of what should be enforced.
> 
>> The hacking repo has consists primarily of two distinct things, HACKING.rst the 
>> OpenStack Style Guide that is published at [0], and hacking the tool. Hacking 
>> the style guide's goal is as Sean puts it [1]:
> 
>>     "OpenStack has a set of style guidelines for clarity. OpenStack is a very
>>     large code base (over 1 Million lines of python), spanning dozens of git
>>     trees, with over a thousand developers contributing every 12 months. As such
>>     common style helps developers understand code in reviews, move between
>>     projects smoothly, and overall make the code more maintainable." 
> 
>> While hacking the tool is there to move the burden of enforcing the style guide 
>> off of human reviewers, as human reviewers are our most constrained resource today.
> 
>> In the past when evaluating if we should add a new rule to hacking the tool, we 
>> followed the  guidelines in [2]. Where consensus was met if the rule was already 
>> in HACKING.rst or there was lazy consensus on this mailing list. In retrospect 
>> this was a mistake, as this policy makes the assumption that the folks read 
>> HACKING.rst and have done a review to decide if any sections should be changed. 
>> For example we have a few unenforced sections that we may not want to keep 
>> [3][4]. Going forward I propose:
> 
>>   * Any addition or removal of a rule requires a ML thread and/or an oslo-specs
>>     blueprint (I am not sure which one makes the most sense here, but I am
>>     leaning towards the ML).
>>   * Only accept new rules that are already being used as a local rule in at
>>     least one repository.
>>   * Add a new directory, contrib, for local rules that multiple projects use but
>>     are not generally considered acceptable to be enabled by default. This way
>>     we can reduce the amount of cut and pasted code (thank you to Ben Nemec for
>>     this idea). 
> 
>> While turning off rules at the repository level has always been recommended as 
>> hacking is a tool to help projects and not a mechanism to dictate style, having 
>> rules enabled in hacking does have implications. So we need a mechanism to track 
>> which rules folks don't find useful so we can make sure they are not enabled by 
>> default (either move them to contrib or remove them entirely). We can track 
>> individual repositories hacking ignore lists and periodically reevaluate the 
>> rules with the most ignores. This means projects vote on which rules to remove 
>> by maintaining there ignore list in tox.ini. I put together a tiny script to do 
>> this and here are my findings so far.
> 
>> rule: number of ignores
> 
>> H803: 19
>> H302: 14
>> H904: 14
>> H305: 13
>> H405: 12
>> H307: 11
>> H404: 9
>> H402: 6
>> H: 4

So the H: excludes really then contribute to other excludes, though
typically they also include specific rules. Are the net result of those
caculated as well?

>> H233: 3
>> H202: 3
>> H306: 2
>> H301: 2
>> H303: 1
>> H703: 1
>> H304: 1
>> H237: 1
>> H4: 1
>> H201: 1
>> H701: 1
>> H102: 1
>> H702: 1
> 
>> Interestingly, of the two rules we just agreed to remove, H402 is not even close to the top of the most skipped list. Of the top 3 most skipped rules
> 
> 
> 
>>   * H803: The first line of the commit message must not end with a period and
>>     must be followed by a single blank line.
>>       o Removing, as previously discussed
>>   * H302: Do not import objects, only modules (*)
>>       o This has been around for a long time, should we move this to contrib?
>>   * H904: Wrap long lines in parentheses and not a backslash for line continuation.
>>       o Although this has been in HACKING.rst for a while, the rule was added in
>>         hacking 0.9. So its still unclear if this is skipped because folks don't
>>         like it or because they haven't gotten around to fixing it up yet.
>>         Thoughts? 
> 
> I personally like H302, but don't mind either way about the other two.
> H302 requires lots of code changes so if you haven't started with it
> it's a bit painful to enable that check (might be the reason people
> just ignore it).

I agree, H302 has been really handy in making code more readable.
Realistically this is one of those things that in Tempest we've told
people not to mass patch it, but that we should fix it as we go along.
It took a long time to get there, but it was good, and not completely
disruptive.

Also, curiously, neutron is H302 / H304 clean ... but has them in the
ignores. I wonder how many other projects are that way.

Having it available in an optional area would be good if it goes off by
default, because we definitely still want this in Tempest

> You could ditch H4*

Agreed.

> Is there a way that "git review -s" could install the H8* rules into
> a git pre commit hook locally instead of as gate checks?

The issue is synchronizing bug fixes. Client installed code kind of
sucks in that regard.

	-Sean

-- 
Sean Dague
http://dague.net

-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 482 bytes
Desc: OpenPGP digital signature
URL: <http://lists.openstack.org/pipermail/openstack-dev/attachments/20140624/ea074dce/attachment.pgp>