<div dir="ltr"><br><div class="gmail_extra"><br><div class="gmail_quote">On Thu, Nov 20, 2014 at 9:49 AM, Sahid Orentino Ferdjaoui <span dir="ltr"><<a href="mailto:sahid.ferdjaoui@redhat.com" target="_blank">sahid.ferdjaoui@redhat.com</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">This is something we can call nitpiking or low priority.<br></blockquote><div><br></div><div>This all seems like nitpicking for very little value. I think there are better things we can be focusing on instead of thinking of new ways to nit pick. So I am -1 on all of these.</div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<br>
I would like we introduce 3 new hacking rules to enforce the cohesion<br>
and consistency in the base code.<br>
<br>
<br>
Using boolean assertions<br>
------------------------<br>
<br>
Some tests are written with equality assertions to validate boolean<br>
conditions which is something not clean:<br>
<br>
  assertFalse([]) asserts an empty list<br>
  assertEqual(False, []) asserts an empty list is equal to the boolean<br>
  value False which is something not correct.<br>
<br>
Some changes has been started here but still needs to be appreciated<br>
by community:<br>
<br>
 * <a href="https://review.openstack.org/#/c/133441/" target="_blank">https://review.openstack.org/#/c/133441/</a><br>
 * <a href="https://review.openstack.org/#/c/119366/" target="_blank">https://review.openstack.org/#/c/119366/</a><br>
<br>
<br>
Using same order of arguments in equality assertions<br>
----------------------------------------------------<br>
<br>
Most of the code is written with assertEqual(Expected, Observed) but<br>
some part are still using the opposite. Even if they provide any real<br>
optimisation using the same convention help reviewing and keep a<br>
better consistency in the code.<br>
<br>
  assertEqual(Expected, Observed) OK<br>
  assertEqual(Observed, Expected) KO<br>
<br>
A change has been started here but still needs to be appreciated by<br>
community:<br>
<br>
 * <a href="https://review.openstack.org/#/c/119366/" target="_blank">https://review.openstack.org/#/c/119366/</a><br>
<br>
<br>
Using LOG.warn instead of LOG.warning<br>
-------------------------------------<br>
<br>
We can see many time reviewers -1ed a patch to ask developer to use<br>
'warn' instead of 'warning'. This will provide no optimisation<br>
but let's finally have something clear about what we have to use.<br>
<br>
  LOG.warning: 74<br>
  LOG.warn:    319<br>
<br>
We probably want to use 'warn'<br>
<br>
Nothing has been started from what I know.<br>
<br>
<br>
Thanks,<br>
s.<br>
<br>
_______________________________________________<br>
OpenStack-dev mailing list<br>
<a href="mailto:OpenStack-dev@lists.openstack.org">OpenStack-dev@lists.openstack.org</a><br>
<a href="http://lists.openstack.org/cgi-bin/mailman/listinfo/openstack-dev" target="_blank">http://lists.openstack.org/cgi-bin/mailman/listinfo/openstack-dev</a><br>
</blockquote></div><br></div></div>