<div dir="ltr"><br><div class="gmail_extra"><br><br><div class="gmail_quote">On Tue, May 6, 2014 at 6:03 AM, Doug Hellmann <span dir="ltr"><<a href="mailto:doug.hellmann@dreamhost.com" target="_blank">doug.hellmann@dreamhost.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 Mon, May 5, 2014 at 9:06 PM, David Stanek <<a href="mailto:dstanek@dstanek.com">dstanek@dstanek.com</a>> wrote:<br>
><br>
> On Mon, May 5, 2014 at 5:28 PM, Doug Hellmann <<a href="mailto:doug.hellmann@dreamhost.com">doug.hellmann@dreamhost.com</a>><br>
> wrote:<br>
>><br>
>><br>
>> > The assert ones do seem to fit the best practices as I understand them,<br>
>> > but<br>
>> > I suspect there's going to be quite a bit of work to get projects<br>
>> > compliant.<br>
>><br>
>> I've seen some work being done on that already, but I don't know how<br>
>> strongly we care about those specific rules as an overall project.<br>
>><br>
><br>
> I created the Keystone blueprint[1] to automate the things we already check<br>
> for in reviews. My motivation was to make it faster for contributors to<br>
> contribute because they would get feedback before getting a bunch of -1s in<br>
> Gerrit. I also wanted to free up core dev resources so that we can focus on<br>
> more important parts of reviews.<br>
<br>
</div>Sure, that makes sense to do in keystone.<br></blockquote><div><br></div><div><br></div><div>While local checks are great, when adding them projects should always think if it makes sense to push them back to hacking itself. Otherwise we end up with 10 different versions of the same checks.</div>
<div><br></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>
><br>
> I'd be happy to start putting some of these in hacking, but I don't know<br>
> which rules would be acceptable to all projects. Maybe there is a way to<br>
> make optional checks that can be enabled in the tox.ini<br></div></blockquote><div><br></div><div>All rules are optional in hacking, but all are enabled by default. If a project doesn't want to use one they can just disable it.</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>
</div>This is the part I wasn't sure about. Some of the rules look pretty<br>
useful (e.g., no mutable arguments). Others are more stylistic choices<br>
(e.g., which type of quotes to use).<br>
<br>
Joe Gordon drives the hacking project, and he might be able to give<br>
more detail about the process for adding new rules there.<br>
<span class=""><font color="#888888"><br>
Doug<br>
</font></span><div class="im"><br>
><br>
> 1.<br>
> <a href="https://blueprints.launchpad.net/keystone/+spec/more-code-style-automation" target="_blank">https://blueprints.launchpad.net/keystone/+spec/more-code-style-automation</a></div></blockquote><div><br></div><div>
<br></div><div>Taking the list from your blueprint, with responses after each.</div><div><br></div><div><p style="margin:0px 0px 1.2em;padding:0px;width:auto;max-width:45em;color:rgb(51,51,51);font-family:sans-serif;font-size:12px;line-height:18px">
</p><ul><li> block comments should have a space after the # (this is already enforced for inline comments)<br></li><ul><li>pep8 1.5.x has this check already, it will come out in hacking 0.9 (which is currently blocked on reviews)</li>
</ul><li> mutables should not be used as default args<br></li><ul><li>sounds like a good thing to add to hacking. Although a clear why this is good is required</li></ul><li> % should not be used in log statements<br></li>
<ul><li>Belongs in Hacking, although we have to make sure 'LOG' is a logger</li></ul><li> assertNone should be used when using None with assertEqual<br></li><ul><li>Why? what is the value here?</li></ul><li> spelling check for comments and docstrings<br>
</li><ul><li>I have thought about adding this a few times, but I am -1 on it. The only way this can work is as a blacklist of words, since developers love to make up there own words.</li><li>Having spelling feedback in a non-voting way would be very useful though</li>
</ul><li> warn if children that change method signature of their parent<br></li><ul><li>Hacking doesn't really do warnings</li></ul><li> methods that just pass are useless and should be deleted<br></li><ul><li>How do these even get in?</li>
</ul><li> warn on try-except that just passes<br></li><ul><li>Hacking doesn't do warnings.</li></ul><li> enforce import ordering and spacing<br></li><ul><li>So I am really sad to see this here. Hacking 0.9 supports this (once again blocked on reviews -- not enough reviewers)</li>
</ul><li> _() should not be used in debug log statements</li><ul><li>Hacking, once again we need to be rigorous in this check when adding to hacking.</li></ul></ul><p></p><p style="margin:0px 0px 1.2em;padding:0px;width:auto;max-width:45em;color:rgb(51,51,51);font-family:sans-serif;font-size:12px;line-height:18px">
I'm sure that as I'm fixing the code to conform to the new checks, I'll identify additional checks to implement.</p><p style="margin:0px 0px 1.2em;padding:0px;width:auto;max-width:45em;color:rgb(51,51,51);font-family:sans-serif;font-size:12px;line-height:18px">
<span style="font-family:arial;font-size:small;line-height:normal;color:rgb(34,34,34)">(stevemar) if I may add some more:</span></p><ul><li>ensure no vim headers<br></li><ul><li>I don't think this one belongs in hacking, as once a project removes all vim headers I don't think they will be re-added. So I see value of having this short term in each project. But not something we want to check for indefinitely </li>
</ul><li>assertEqual(len(some_list), 0) should be assertEqual(some_list, [])<br></li><ul><li>Why is this better?</li></ul><li>methods / functions that are unused<br></li><ul><li>This isn't really something hacking can do.</li>
</ul><li>class properties that are unused<br></li><ul><li>Once again this isn't something hacking can really do, or really any static analysis in python. pyflakes tries to do some of this.</li></ul><li>ensure incoming method args are actually used</li>
<ul><li>belongs in Pyflakes</li></ul><li>doc strings - ensure all method args are mentioned (if any are mentioned)<br></li><ul><li>I have to double check, but I think there is a docstring module for flake8 that can do things like this</li>
</ul><li>use ' over "<br></li><ul><li>Why? And is this a black and white thing that can be enforced.</li></ul><li>ensure we don't go assigning copyrights to OpenStack Foundation<br></li><ul><li>We cannot gate on this, otherwise foundation employees cannot commit code.</li>
</ul></ul><p></p></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="im"><br>
><br>
> --<br>
> 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><br>
> www: <a href="http://dstanek.com" target="_blank">http://dstanek.com</a><br>
><br>
</div><div class=""><div class="h5">> _______________________________________________<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>
><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>
</div></div></blockquote></div><br></div></div>