On 27 January 2014 10:10, Daniel P. Berrange <berra...@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 >> <berra...@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. John _______________________________________________ OpenStack-dev mailing list OpenStack-dev@lists.openstack.org http://lists.openstack.org/cgi-bin/mailman/listinfo/openstack-dev
