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