Open Stack

On Thu, Jan 30, 2014 at 2:06 AM, Daniel P. Berrange <berrange at redhat.com> wrote:
> On Wed, Jan 29, 2014 at 01:22:59PM -0500, Joe Gordon wrote:
>> On Tue, Jan 28, 2014 at 4:45 AM, John Garbutt <john at johngarbutt.com> wrote:
>> > On 27 January 2014 10:10, Daniel P. Berrange <berrange at redhat.com> wrote:
>> >> 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.
>> >
>> > +1
>> >
>> > I worry about diverging, but I guess thats always going to happen 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.
>> >>> >
>> >>>
>> >>> 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.
>> >
>> > +1
>> >
>> > I would love to enforce it 100% of the time, but sometimes its hard to
>> > write the rules, but still a useful cleanup. Lets see how it goes I
>> > guess.
>>
>> I am weary of adding any new style rules that have to manually
>> enforced by human reviewers, we already have a lot of other items to
>> cover in a review.
>
> A recent style cleanup was against config variable help strings.
> One of the rules used was "Write complete sentances". This is a
> perfectly reasonable style cleanup, but I challenge anyone to write
> a hacking check that validates "Write complete sentances" in an
> acceptable amount of code. Being pragmatic on when hacking checks
> are needed is the only pratical approach.

Although it would be hard to write a rule to enforce complete
sentences, looking for proper punctuation at the end of the sentence
and a capital letter at the beginning gets us very far.

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