On Thu, Jan 30, 2014 at 2:06 AM, Daniel P. Berrange <[email protected]> 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 <[email protected]> wrote: >> > On 27 January 2014 10:10, Daniel P. Berrange <[email protected]> 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 >> >>> <[email protected]>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 :| _______________________________________________ OpenStack-dev mailing list [email protected] http://lists.openstack.org/cgi-bin/mailman/listinfo/openstack-dev
