<div dir="ltr"><br><div class="gmail_extra"><br><br><div class="gmail_quote">On Fri, Jan 24, 2014 at 7:24 AM, Daniel P. Berrange <span dir="ltr"><<a href="mailto:berrange@redhat.com" target="_blank">berrange@redhat.com</a>></span> wrote:<br>
<blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex">Periodically I've seen people submit big coding style cleanups to Nova<br>
code. These are typically all good ideas / beneficial, however, I have<br>
rarely (perhaps even never?) seen the changes accompanied by new hacking<br>
check rules.<br>
<br>
The problem with not having a hacking check added *in the same commit*<br>
as the cleanup is two-fold<br>
<br>
- No guarantee that the cleanup has actually fixed all violations<br>
in the codebase. Have to trust the thoroughness of the submitter<br>
or do a manual code analysis yourself as reviewer. Both suffer<br>
from human error.<br>
<br>
- Future patches will almost certainly re-introduce the same style<br>
problems again and again and again and again and again and again<br>
and again and again and again.... I could go on :-)<br>
<br>
I don't mean to pick on one particular person, since it isn't their<br>
fault that reviewers have rarely/never encouraged people to write<br>
hacking rules, but to show one example.... The following recent change<br>
updates all the nova config parameter declarations cfg.XXXOpt(...) to<br>
ensure that the help text was consistently styled:<br>
<br>
<a href="https://review.openstack.org/#/c/67647/" target="_blank">https://review.openstack.org/#/c/67647/</a><br>
<br>
One of the things it did was to ensure that the help text always started<br>
with a capital letter. Some of the other things it did were more subtle<br>
and hard to automate a check for, but an 'initial capital letter' rule<br>
is really straightforward.<br>
<br>
By updating nova/hacking/checks.py to add a new rule for this, it was<br>
found that there were another 9 files which had incorrect capitalization<br>
of their config parameter help. So the hacking rule addition clearly<br>
demonstrates its value here.<br>
<br></blockquote><div><br></div><div>This sounds like a rule that we should add to <a href="https://github.com/openstack-dev/hacking.git" target="_blank">https://github.com/openstack-dev/hacking.git</a>.</div><div> </div>
<blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex">
I will concede that documentation about /how/ to write hacking checks<br>
is not entirely awesome. My current best advice is to look at how some<br>
of the existing hacking checks are done - find one that is checking<br>
something that is similar to what you need and adapt it. There are a<br>
handful of Nova specific rules in nova/hacking/checks.py, and quite a<br>
few examples in the shared repo <a href="https://github.com/openstack-dev/hacking.git" target="_blank">https://github.com/openstack-dev/hacking.git</a><br>
see the file hacking/core.py. There's some very minimal documentation<br>
about variables your hacking check method can receive as input<br>
parameters <a href="https://github.com/jcrocholl/pep8/blob/master/docs/developer.rst" target="_blank">https://github.com/jcrocholl/pep8/blob/master/docs/developer.rst</a><br>
<br>
<br>
In summary, if you are doing a global coding style cleanup in Nova for<br>
something which isn't already validated by pep8 checks, then I strongly<br>
encourage additions to nova/hacking/checks.py to validate the cleanup<br>
correctness. Obviously with some style cleanups, it will be too complex<br>
to write logic rules to reliably validate code, so this isn't a code<br>
review point that must be applied 100% of the time. Reasonable personal<br>
judgement should apply. I will try comment on any style cleanups I see<br>
where I think it is pratical to write a hacking check.<br></blockquote><div><br></div><div>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.</div>
<div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex">
<br>
Regards,<br>
Daniel<br>
<span class=""><font color="#888888">--<br>
|: <a href="http://berrange.com" target="_blank">http://berrange.com</a> -o- <a href="http://www.flickr.com/photos/dberrange/" target="_blank">http://www.flickr.com/photos/dberrange/</a> :|<br>
|: <a href="http://libvirt.org" target="_blank">http://libvirt.org</a> -o- <a href="http://virt-manager.org" target="_blank">http://virt-manager.org</a> :|<br>
|: <a href="http://autobuild.org" target="_blank">http://autobuild.org</a> -o- <a href="http://search.cpan.org/~danberr/" target="_blank">http://search.cpan.org/~danberr/</a> :|<br>
|: <a href="http://entangle-photo.org" target="_blank">http://entangle-photo.org</a> -o- <a href="http://live.gnome.org/gtk-vnc" target="_blank">http://live.gnome.org/gtk-vnc</a> :|<br>
<br>
_______________________________________________<br>
OpenStack-dev mailing list<br>
<a href="mailto:OpenStack-dev@lists.openstack.org">OpenStack-dev@lists.openstack.org</a><br>
<a href="http://lists.openstack.org/cgi-bin/mailman/listinfo/openstack-dev" target="_blank">http://lists.openstack.org/cgi-bin/mailman/listinfo/openstack-dev</a><br>
</font></span></blockquote></div><br></div></div>