Open Stack

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