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