Open Stack

On Fri, Jan 24, 2014 at 7:24 AM, Daniel P. Berrange <berrange at 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 at lists.openstack.org
> http://lists.openstack.org/cgi-bin/mailman/listinfo/openstack-dev
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.openstack.org/pipermail/openstack-dev/attachments/20140124/13ca02f8/attachment.html>