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. 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. 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
