On Wed, Jan 29, 2014 at 01:22:59PM -0500, 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.
A recent style cleanup was against config variable help strings. One of the rules used was "Write complete sentances". This is a perfectly reasonable style cleanup, but I challenge anyone to write a hacking check that validates "Write complete sentances" in an acceptable amount of code. Being pragmatic on when hacking checks are needed is the only pratical approach. Regards, Daniel -- |: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :| |: http://libvirt.org -o- http://virt-manager.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: http://entangle-photo.org -o- http://live.gnome.org/gtk-vnc :| _______________________________________________ OpenStack-dev mailing list OpenStack-dev@lists.openstack.org http://lists.openstack.org/cgi-bin/mailman/listinfo/openstack-dev
