Re: [openstack-dev] Nova style cleanups with associated hacking check addition
On Thu, Jan 30, 2014 at 2:06 AM, Daniel P. Berrange 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 wrote: >> > On 27 January 2014 10:10, Daniel P. Berrange 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 >> >>> 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 cle
Re: [openstack-dev] Nova style cleanups with associated hacking check addition
On Wed, Jan 29, 2014 at 01:22:59PM -0500, Joe Gordon wrote: > On Tue, Jan 28, 2014 at 4:45 AM, John Garbutt wrote: > > On 27 January 2014 10:10, Daniel P. Berrange 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 > >>> 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
Re: [openstack-dev] Nova style cleanups with associated hacking check addition
On 01/29/2014 07:22 PM, Joe Gordon wrote: > On Tue, Jan 28, 2014 at 4:45 AM, John Garbutt wrote: >> On 27 January 2014 10:10, Daniel P. Berrange 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 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. Based on the feedback I got on IRC, I've written such a style guide a few days ago for the config help strings: https://review.openstack.org/#/c/69381 Even if you do not notice these during a review, it's easy for somebody else to cleanup - and then it's important to have a style guide that explains how things should be written. I'd like to have consistency across OpenS
Re: [openstack-dev] Nova style cleanups with associated hacking check addition
On Tue, Jan 28, 2014 at 4:45 AM, John Garbutt wrote: > On 27 January 2014 10:10, Daniel P. Berrange 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 >>> 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. > > John ___ OpenStack-dev mailing list OpenStack-dev@lists.openstack.org http://lists.openstack.org/cgi-bin/mailman/listinfo/openstack-dev
Re: [openstack-dev] Nova style cleanups with associated hacking check addition
On 27 January 2014 10:10, Daniel P. Berrange 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 >> 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
Re: [openstack-dev] Nova style cleanups with associated hacking check addition
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 > 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 :| ___ OpenStack-dev mailing list OpenStack-dev@lists.openstack.org http://lists.openstack.org/cgi-bin/mailman/listinfo/openstack-dev
Re: [openstack-dev] Nova style cleanups with associated hacking check addition
On Fri, Jan 24, 2014 at 7:24 AM, Daniel P. Berrange 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. > 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. > > 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 > OpenStack-dev@lists.openstack.org > http://lists.openstack.org/cgi-bin/mailman/listinfo/openstack-dev > ___ OpenStack-dev mailing list OpenStack-dev@lists.openstack.org http://lists.openstack.org/cgi-bin/mailman/listinfo/openstack-dev
[openstack-dev] Nova style cleanups with associated hacking check addition
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. 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. 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 OpenStack-dev@lists.openstack.org http://lists.openstack.org/cgi-bin/mailman/listinfo/openstack-dev
