On 01/29/2014 07:22 PM, Joe Gordon wrote: > On Tue, Jan 28, 2014 at 4:45 AM, John Garbutt <j...@johngarbutt.com> wrote: >> On 27 January 2014 10:10, Daniel P. Berrange <berra...@redhat.com> wrote: >>> On Fri, Jan 24, 2014 at 11:42:54AM -0500, Joe Gordon wrote: >>>> On Fri, Jan 24, 2014 at 7:24 AM, Daniel P. Berrange >>>> <berra...@redhat.com>wrote: >>>> >>>>> Periodically I've seen people submit big coding style cleanups to Nova >>>>> code. These are typically all good ideas / beneficial, however, I have >>>>> rarely (perhaps even never?) seen the changes accompanied by new hacking >>>>> check rules. >>>>> >>>>> The problem with not having a hacking check added *in the same commit* >>>>> as the cleanup is two-fold >>>>> >>>>> - No guarantee that the cleanup has actually fixed all violations >>>>> in the codebase. Have to trust the thoroughness of the submitter >>>>> or do a manual code analysis yourself as reviewer. Both suffer >>>>> from human error. >>>>> >>>>> - Future patches will almost certainly re-introduce the same style >>>>> problems again and again and again and again and again and again >>>>> and again and again and again.... I could go on :-) >>>>> >>>>> I don't mean to pick on one particular person, since it isn't their >>>>> fault that reviewers have rarely/never encouraged people to write >>>>> hacking rules, but to show one example.... The following recent change >>>>> updates all the nova config parameter declarations cfg.XXXOpt(...) to >>>>> ensure that the help text was consistently styled: >>>>> >>>>> https://review.openstack.org/#/c/67647/ >>>>> >>>>> One of the things it did was to ensure that the help text always started >>>>> with a capital letter. Some of the other things it did were more subtle >>>>> and hard to automate a check for, but an 'initial capital letter' rule >>>>> is really straightforward. >>>>> >>>>> By updating nova/hacking/checks.py to add a new rule for this, it was >>>>> found that there were another 9 files which had incorrect capitalization >>>>> of their config parameter help. So the hacking rule addition clearly >>>>> demonstrates its value here. >>>> >>>> This sounds like a rule that we should add to >>>> https://github.com/openstack-dev/hacking.git. >>> >>> Yep, it could well be added there. I figure rules added to Nova can >>> be "upstreamed" to the shared module periodically. >> >> +1 >> >> I worry about diverging, but I guess thats always going to happen here. >> >>>>> I will concede that documentation about /how/ to write hacking checks >>>>> is not entirely awesome. My current best advice is to look at how some >>>>> of the existing hacking checks are done - find one that is checking >>>>> something that is similar to what you need and adapt it. There are a >>>>> handful of Nova specific rules in nova/hacking/checks.py, and quite a >>>>> few examples in the shared repo >>>>> https://github.com/openstack-dev/hacking.git >>>>> see the file hacking/core.py. There's some very minimal documentation >>>>> about variables your hacking check method can receive as input >>>>> parameters >>>>> https://github.com/jcrocholl/pep8/blob/master/docs/developer.rst >>>>> >>>>> >>>>> In summary, if you are doing a global coding style cleanup in Nova for >>>>> something which isn't already validated by pep8 checks, then I strongly >>>>> encourage additions to nova/hacking/checks.py to validate the cleanup >>>>> correctness. Obviously with some style cleanups, it will be too complex >>>>> to write logic rules to reliably validate code, so this isn't a code >>>>> review point that must be applied 100% of the time. Reasonable personal >>>>> judgement should apply. I will try comment on any style cleanups I see >>>>> where I think it is pratical to write a hacking check. >>>>> >>>> >>>> I would take this even further, I don't think we should accept any style >>>> cleanup patches that can be enforced with a hacking rule and aren't. >>> >>> IMHO that would mostly just serve to discourage people from submitting >>> style cleanup patches because it is too much stick, not enough carrot. >>> Realistically for some types of style cleanup, the effort involved in >>> writing a style checker that does not have unacceptable false positives >>> will be too high to justify. So I think a pragmatic approach to enforcement >>> is more suitable. >> >> +1 >> >> I would love to enforce it 100% of the time, but sometimes its hard to >> write the rules, but still a useful cleanup. Lets see how it goes I >> guess. > > I am weary of adding any new style rules that have to manually > enforced by human reviewers, we already have a lot of other items to > cover in a review.
Based on the feedback I got on IRC, I've written such a style guide a few days ago for the config help strings: https://review.openstack.org/#/c/69381 Even if you do not notice these during a review, it's easy for somebody else to cleanup - and then it's important to have a style guide that explains how things should be written. I'd like to have consistency across OpenStack on how these help strings are written. Andreas -- Andreas Jaeger aj@{suse.com,opensuse.org} Twitter/Identica: jaegerandi SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany GF: Jeff Hawn,Jennifer Guild,Felix Imendörffer,HRB16746 (AG Nürnberg) GPG fingerprint = 93A3 365E CE47 B889 DF7F FED1 389A 563C C272 A126 _______________________________________________ OpenStack-dev mailing list OpenStack-dev@lists.openstack.org http://lists.openstack.org/cgi-bin/mailman/listinfo/openstack-dev