<div dir="ltr"><br><div class="gmail_extra"><br><div class="gmail_quote">On Tue, Jan 12, 2016 at 8:51 AM, Amrith Kumar <span dir="ltr"><<a href="mailto:amrith@tesora.com" target="_blank">amrith@tesora.com</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">I've tagged this message with the projects impacted by a series of change sets:<br>
<br>
[trove] <a href="https://review.openstack.org/#/c/266220/" rel="noreferrer" target="_blank">https://review.openstack.org/#/c/266220/</a><br>
[neutron] <a href="https://review.openstack.org/#/c/266156/1" rel="noreferrer" target="_blank">https://review.openstack.org/#/c/266156/1</a><br>
[cinder] <a href="https://review.openstack.org/#/c/266099/2" rel="noreferrer" target="_blank">https://review.openstack.org/#/c/266099/2</a><br>
[swift] <a href="https://review.openstack.org/#/c/266185/1" rel="noreferrer" target="_blank">https://review.openstack.org/#/c/266185/1</a><br>
[ceilometer] <a href="https://review.openstack.org/#/c/266211/1" rel="noreferrer" target="_blank">https://review.openstack.org/#/c/266211/1</a><br>
[nova] <a href="https://review.openstack.org/#/c/266143/2" rel="noreferrer" target="_blank">https://review.openstack.org/#/c/266143/2</a><br>
[keystone] <a href="https://review.openstack.org/#/c/266203/2" rel="noreferrer" target="_blank">https://review.openstack.org/#/c/266203/2</a><br>
[sahara] <a href="https://review.openstack.org/#/c/266230/1" rel="noreferrer" target="_blank">https://review.openstack.org/#/c/266230/1</a><br>
[glance] <a href="https://review.openstack.org/#/c/266192/1" rel="noreferrer" target="_blank">https://review.openstack.org/#/c/266192/1</a><br>
[neutron-lbaas] <a href="https://review.openstack.org/#/c/266181/1" rel="noreferrer" target="_blank">https://review.openstack.org/#/c/266181/1</a><br>
<br>
I would like the guidance of the developer community in figuring out how to proceed with this change, and changes like this.<br>
<br>
The change, in essence changes a construct of the kind:<br>
<br>
if var > 0:<br>
... something ...<br>
<br>
To<br>
<br>
if var:<br>
... something ...<br>
<br>
In a couple of cases, it also changes messages from (for example, in Trove)<br>
<br>
"Limit value must be > 0" to<br>
"Limit value must be greater than zero"<br>
<br>
My question to the ML is this, should stylistic changes of this kind be handled in a consistent way across all projects, maybe with a hacking rule and some discussion on the ML first? After all, if this change is worthwhile, it is worth ensuring that this construct that we are seeking to eliminate, does not reenter the code base.<br></blockquote><div><br></div><div>The code above is not stylistic in nature. It actually changes the semantics of the check. This should not be done blindly across the board.</div><div><br></div><div>In the first case it looks for a value to be greater than a constant. In the second case it looks for a value to have a truthy value.</div><div><br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<br>
For what it is worth, I agree with Vitaly Grindev [sahara, in review <a href="https://review.openstack.org/#/c/266230/1" rel="noreferrer" target="_blank">https://review.openstack.org/#/c/266230/1</a>]. I think the code before the change was more intuitive and readable.<br>
</blockquote></div><br>Only if the code is correct. In that review I saw you changing "len(x) > 0" to "len(x)" and that's fine. But you also changed a check looking at the status code from a conductor call to no longer allow negative return values. I have no idea if it returns negative numbers, but in this case I would guess the "> 0" had a purpose.<br clear="all"><div><br></div>-- <br><div class="gmail_signature">David<br>blog: <a href="http://www.traceback.org" target="_blank">http://www.traceback.org</a><br>twitter: <a href="http://twitter.com/dstanek" target="_blank">http://twitter.com/dstanek</a><div>www: <a href="http://dstanek.com" target="_blank">http://dstanek.com</a></div></div>
</div></div>