<div dir="ltr"><br><div class="gmail_extra"><br><br><div class="gmail_quote">On Mon, May 5, 2014 at 10:56 AM, Ben Nemec <span dir="ltr"><<a href="mailto:openstack@nemebean.com" target="_blank">openstack@nemebean.com</a>></span> wrote:<br>
<blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex"><div class="">On 05/05/2014 10:02 AM, ChangBo Guo wrote:<br>
<blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex">
Hi Stackers,<br>
<br>
I find some common code style would be avoided while I'm reviewing code<br>
,so think these check would<br>
be nice to move into local hacking. The local hacking can ease reviewer<br>
burden.<br>
<br>
The idea from keystone blueprint [1].<br>
Hacking is a great start at automating checks for common style issues.<br>
There are still lots of things that it is not checking for that it<br>
probably should. The local hacking ease reviewer burden . This is the<br>
list of from [1][2] that would be nice to move into an automated check:<br>
<br>
- use import style 'from openstack.common.* import' not use 'import<br>
openstack.common.*'<br>
</blockquote>
<br></div>
This is the only one that I think belongs in Oslo. The others are all generally applicable, but the other projects aren't going to want to enforce the import style since it's only to make Oslo syncs work right.<div class="">
<br></div></blockquote><div><br></div><div>I agree this belongs in oslo-incubator only as well. But I am still confused as to why this is needed. It sounds like this is a workaround for a bug in the sync script.</div><div>
</div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex"><div class="">
<br>
<blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex">
- assertIsNone should be used when using None with assertEqual<br>
- _() should not be used in debug log statements<br>
-do not use 'assertTrue(isinstance(a, b)) sentence'<br>
-do not use 'assertEqual(type(A), B) sentence'<br>
</blockquote>
<br></div>
The _() one in particular I think we'll want as we make the logging changes. Some additional checks to make sure the the correct _ function is used with the correct logging function would be good too (for example, LOG.warning(_LE('foo')) should fail pep8).<br>
<br>
But again, that belongs in hacking proper, not an Oslo module.<br></blockquote><div><br></div><div><br></div><div>Yup, this belongs in hacking although this gets a little tricky to do right, as we don't want false positives <a href="http://git.openstack.org/cgit/openstack-dev/hacking/tree/README.rst#n42">http://git.openstack.org/cgit/openstack-dev/hacking/tree/README.rst#n42</a></div>
<div><br></div><div>We have to make sure that '_' is what we think it is and that 'LOG' is the logger, just checking for 'LOG' isn't very robust.</div><div><br></div><div>Patches welcome! <a href="http://git.openstack.org/cgit/openstack-dev/hacking">http://git.openstack.org/cgit/openstack-dev/hacking</a></div>
<div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex">
<br>
The assert ones do seem to fit the best practices as I understand them, but I suspect there's going to be quite a bit of work to get projects compliant.<br></blockquote><div><br></div><div>Although some projects already have the assert ones, I don't see a lot of value in them.</div>
<div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex">
<br>
<blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex"><div class="">
<br>
[1]<br>
<a href="https://blueprints.launchpad.net/keystone/+spec/more-code-style-automation" target="_blank">https://blueprints.launchpad.<u></u>net/keystone/+spec/more-code-<u></u>style-automation</a><br>
[2] <a href="https://github.com/openstack/nova/blob/master/nova/hacking/checks.py" target="_blank">https://github.com/openstack/<u></u>nova/blob/master/nova/hacking/<u></u>checks.py</a><br>
<br>
I just registered a blueprint for this in [3] and submit first patch in [4].<br>
<br>
[3] <a href="https://blueprints.launchpad.net/oslo/+spec/oslo-local-hacking" target="_blank">https://blueprints.launchpad.<u></u>net/oslo/+spec/oslo-local-<u></u>hacking</a><br>
<br>
[4] <a href="https://review.openstack.org/#/c/87832/" target="_blank">https://review.openstack.org/#<u></u>/c/87832/</a><br></div>
<<a href="https://github.com/openstack/nova/blob/master/nova/hacking/checks.py" target="_blank">https://github.com/openstack/<u></u>nova/blob/master/nova/hacking/<u></u>checks.py</a>><div class=""><br>
<br>
Should we add local hacking for oslo-incubator ? If yes, what's the<br>
other check will be added ?<br>
Your comment is appreciated :-)<br>
<br>
--<br>
ChangBo Guo(gcb)<br>
<br>
<br></div><div class="">
______________________________<u></u>_________________<br>
OpenStack-dev mailing list<br>
<a href="mailto:OpenStack-dev@lists.openstack.org" target="_blank">OpenStack-dev@lists.openstack.<u></u>org</a><br>
<a href="http://lists.openstack.org/cgi-bin/mailman/listinfo/openstack-dev" target="_blank">http://lists.openstack.org/<u></u>cgi-bin/mailman/listinfo/<u></u>openstack-dev</a><br>
<br>
</div></blockquote><div class=""><div class="h5">
<br>
<br>
______________________________<u></u>_________________<br>
OpenStack-dev mailing list<br>
<a href="mailto:OpenStack-dev@lists.openstack.org" target="_blank">OpenStack-dev@lists.openstack.<u></u>org</a><br>
<a href="http://lists.openstack.org/cgi-bin/mailman/listinfo/openstack-dev" target="_blank">http://lists.openstack.org/<u></u>cgi-bin/mailman/listinfo/<u></u>openstack-dev</a><br>
</div></div></blockquote></div><br></div></div>