Open Stack

On Fri, Jan 24, 2014 at 11:42:54AM -0500, Joe Gordon wrote:
> 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.

Yep, it could well be added there. I figure rules added to Nova can
be "upstreamed" to the shared module periodically.

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

IMHO that would mostly just serve to discourage people from submitting
style cleanup patches because it is too much stick, not enough carrot.
Realistically for some types of style cleanup, the effort involved in
writing a style checker that does not have unacceptable false positives
will be too high to justify. So I think a pragmatic approach to enforcement
is more suitable.

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