<div dir="ltr"><div class="gmail_extra"><div class="gmail_quote">Answers to the specifics of this thread here, and I will follow up with a seperate thread on the broader topic.</div><div class="gmail_quote"><br></div><div class="gmail_quote">
On Mon, Jun 23, 2014 at 7:21 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">
-----BEGIN PGP SIGNED MESSAGE-----<br>
Hash: SHA1<br>
<div class=""><br>
On 06/23/2014 06:18 AM, Sean Dague wrote:<br>
> On 06/22/2014 03:04 PM, Jay Pipes wrote:<br>
>> On 06/20/2014 02:07 PM, Sean Dague wrote:<br>
>>> After seeing a bunch of code changes to enforce new hacking<br>
>>> rules, I'd like to propose dropping some of the rules we have.<br>
>>> The overall patch series is here -<br>
>>> <a href="https://review.openstack.org/#/q/status:open+project:openstack-dev/hacking+branch:master+topic:be_less_silly,n,z" target="_blank">https://review.openstack.org/#/q/status:open+project:openstack-dev/hacking+branch:master+topic:be_less_silly,n,z</a><br>
>>><br>
>>><br>
>>><br>
>>><br>
H402 - 1 line doc strings should end in punctuation. The real statement<br>
>>> is this should be a summary sentence. A sentence is not just a<br>
>>> set of words that end in a period. Squirel fast bob. It's<br>
>>> something deeper. This rule thus isn't really semantically<br>
>>> useful, especially when you are talking about at 69 character<br>
>>> maximum (79 - 4 space indent - 6 quote characters).<br>
>><br>
>> ++ This was always a silly rule, IMO<br>
>><br>
>> Sorry, forgot to add a period. There. Better.<br></div></blockquote><div><br></div><div>After looking at <a href="http://legacy.python.org/dev/peps/pep-0257/">http://legacy.python.org/dev/peps/pep-0257/</a> it says </div>
<div><span style="line-height:1.5;color:rgb(0,0,0);font-family:Arial,Verdana,Geneva,'Bitstream Vera Sans',Helvetica,sans-serif;font-size:15px"><br></span></div><div> <span style="line-height:1.5;color:rgb(0,0,0);font-family:Arial,Verdana,Geneva,'Bitstream Vera Sans',Helvetica,sans-serif;font-size:15px">The docstring is a phrase ending in a period. It prescribes the function or method's effect as a command ("Do this", "Return that"), not as a description; e.g. don't write "Returns the pathname ...".'</span></div>
<div><br></div><div><br></div><div>So it looks like pep257 doesn't agree with you, and I am confused at pep257 itself.</div><div><br></div><div>That being said since most folks aren't a fan, I am for removing it.</div>
<div> <br></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>
>>> H803 - First line of a commit message must *not* end in a<br>
>>> period. This was mostly a response to an unreasonable core<br>
>>> reviewer that was -1ing people for not having periods. I think<br>
>>> any core reviewer that -1s for this either way should be thrown<br>
>>> off the island, or at least made fun of, a lot. Again, the<br>
>>> clarity of a commit message is not made or lost by the lack or<br>
>>> existence of a period at the end of the first line.<br>
>><br>
>> +10<br>
>><br>
>> Sorry, I meant +10. Period.<br>
>><br>
<br>
</div>I'm good with removing the punctuation ones, especially for the commit<br>
message.<br></blockquote><div><br></div><div><br></div><div>I think there is a fairly strong concensus on removing H803, it should have never landed in the first place.</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>
>>> H305 - Enforcement of libraries fitting correctly into stdlib,<br>
>>> 3rdparty, our tree. This biggest issue here is it's built in a<br>
>>> world where there was only 1 viable python version, 2.7.<br>
>>> Python's stdlib is actually pretty dynamic and grows over time.<br>
>>> As we embrace more python 3, and as distros start to make<br>
>>> python3 be front and center, what does this even mean? The<br>
>>> current enforcement can't pass on both python2 and python3 at<br>
>>> the same time in many cases because of that.<br>
>><br>
>> +0 on this one... I find it useful to group the libs together in<br>
>> this way, as it makes it relatively easy to identify quickly at a<br>
>> glance the third party libs that are in use in the module.<br>
><br>
> Sure, I'm not saying don't group. I'm saying that it's actually<br>
> impossible to create a formal definition for group 2. So use it as<br>
> guidelines don't enforce in code.<br>
<br>
</div>I'm -1 on manual enforcement of these rules. I hate -1'ing a patch<br>
(especially one that's been in review for a while) just for import<br>
grouping issues, and if I'm not going to -1 for that then why even<br>
pretend it's something we want to do?<br>
<br>
I'd rather have an exception mechanism for the check where we can make<br>
a list of modules that can be in either stdlib or third-party and<br>
allow both. I don't think the Python stdlib is changing so fast that<br>
we couldn't keep such a list maintained and it would allow these<br>
checks to still be automated. In fact, it should be possible to<br>
generate the list programmatically.<br>
<br>
Obviously I am signing up to do this work if it's something we can<br>
agree on.<br>
<div><div class="h5"><br></div></div></blockquote><div><br></div><div><br></div><div>Sounds like Ben just volunteered to fix this one up, thank you. So given Ben's offer to fix this up I don't think we should remove 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><div class="h5">
><br>
>>> We have to remember we're all humans, and it's ok to have grey<br>
>>> space. Like in 305, you *should* group the libraries if you<br>
>>> can, but stuff like that should be labeled as 'nit' in the<br>
>>> review, and only ask the author to respin it if there are other<br>
>>> more serious issues to be handled.<br>
>><br>
>> Ya, no real disagreement on that.<br>
>><br>
>>> Let's optimize a little more for fun, and stop throwing -1s for<br>
>>> silly things. :)<br>
>><br>
>> ++<br>
>><br>
>> I would also love to get rid of H404, otherwise known as the dumb<br>
>> rule that says if you have a multiline docstring, that there must<br>
>> be a summary line, then a blank line, then a detailed<br>
>> description. It makes things like this illegal, which, IMHO, is<br>
>> stupid:<br>
>><br>
>> def do_something(self, thing): """We do something with the<br>
>> supplied thing, so that something else is also done with this<br>
>> other thing. Make sure the other thing is of type something. """<br>
>> pass<br>
>><br>
>> Likewise, I'd love to be able to have a newline start the<br>
>> docstring, like so:<br>
>><br>
>> def do_something(self, thing): """ We do something with the<br>
>> supplied thing, so that something else is also done with this<br>
>> other thing. Make sure the other thing is of type something. """<br>
>> pass<br>
>><br>
>> But there's a rule that prevents that as well...<br>
><br>
> Yeh, I'd be happy throwing out the docstring rules all together. I<br>
> feel like docstrings being good or bad is pretty orthoginal to<br>
> these rules the way they enforce things.<br>
><br>
>> To be clear, I don't think all hacking rules are silly. To the<br>
>> contrary, there are many that are reasonable and useful. However,<br>
>> I'd prefer to focus on things that make the code more readable,<br>
>> not less readable, and rules that enforce Pythonic idioms, not<br>
>> some random hacker's idea of good style.<br>
><br>
> Correct, I'm with you. But I also feel like we've wandered across a<br>
> line in the current implementation, and it's time to prune back.<br>
><br>
> Because hacking supports local rules, if any particular project<br>
> wants to go overboard in rules, so be it. But lets not do that in<br>
> hacking itself.<br>
<br>
</div></div>I made this comment on one of the reviews too, but it would be nice if<br>
we had a central repository for these sorts of checks. Otherwise<br>
we're going to end up with any project that wants to enforce, say, the<br>
docstring checks having to rewrite the checks in their local repo and<br>
there will be a bunch of different implementations of the same rule.<br>
<br>
><br>
> -Sean<br>
<div class="">><br>
><br>
><br>
> _______________________________________________ OpenStack-dev<br>
> mailing list <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>
</div>-----BEGIN PGP SIGNATURE-----<br>
Version: GnuPG v1<br>
Comment: Using GnuPG with Thunderbird - <a href="http://www.enigmail.net/" target="_blank">http://www.enigmail.net/</a><br>
<br>
iQEcBAEBAgAGBQJTqDfxAAoJEDehGd0Fy7uqm9UIAIsb8Pk2yyLqGzrUnPRWZNLZ<br>
nlLoKDwMIu6iQmP6CYuGNZtxse4anUZN0Xzm5q/E90S1jyDRP4uHCTrw6xwNaQ69<br>
yauP37Y4pA18rv/zjABpckv/USyL0vantWOXPxnQEkZsRIUF5Lh0zehI9AkIVlwy<br>
PfWudRX87wIfXQTjaLRW4otFt5D3udqIfinLS9LBK+2YlpZZ95htTMLBI0ZMzat8<br>
5qVwPFrZ/tOW7DQmAzisYQ7C46oB9DLB2cehWZgoEk0dkLrBiEPOojRoHpPK3s0J<br>
fIaTe/PLY5t9wSMxxJehLwkpFcu1M2CdLomLtuspXEjPpzmSsnDtSWjeMsgvMjs=<br>
=jLGp<br>
-----END PGP SIGNATURE-----<br>
<div class=""><div class="h5"><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>