Open Stack

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

On 06/23/2014 06:18 AM, Sean Dague wrote:
> On 06/22/2014 03:04 PM, Jay Pipes wrote:
>> On 06/20/2014 02:07 PM, Sean Dague wrote:
>>> After seeing a bunch of code changes to enforce new hacking
>>> rules, I'd like to propose dropping some of the rules we have.
>>> The overall patch series is here - 
>>> https://review.openstack.org/#/q/status:open+project:openstack-dev/hacking+branch:master+topic:be_less_silly,n,z
>>>
>>>
>>>
>>> 
H402 - 1 line doc strings should end in punctuation. The real statement
>>> is this should be a summary sentence. A sentence is not just a
>>> set of words that end in a period. Squirel fast bob. It's
>>> something deeper. This rule thus isn't really semantically
>>> useful, especially when you are talking about at 69 character
>>> maximum (79 - 4 space indent - 6 quote characters).
>> 
>> ++ This was always a silly rule, IMO
>> 
>> Sorry, forgot to add a period. There. Better.
>> 
>>> H803 - First line of a commit message must *not* end in a
>>> period. This was mostly a response to an unreasonable core
>>> reviewer that was -1ing people for not having periods. I think
>>> any core reviewer that -1s for this either way should be thrown
>>> off the island, or at least made fun of, a lot. Again, the
>>> clarity of a commit message is not made or lost by the lack or
>>> existence of a period at the end of the first line.
>> 
>> +10
>> 
>> Sorry, I meant +10. Period.
>> 

I'm good with removing the punctuation ones, especially for the commit
message.

>>> H305 - Enforcement of libraries fitting correctly into stdlib,
>>> 3rdparty, our tree. This biggest issue here is it's built in a
>>> world where there was only 1 viable python version, 2.7.
>>> Python's stdlib is actually pretty dynamic and grows over time.
>>> As we embrace more python 3, and as distros start to make
>>> python3 be front and center, what does this even mean? The
>>> current enforcement can't pass on both python2 and python3 at 
>>> the same time in many cases because of that.
>> 
>> +0 on this one... I find it useful to group the libs together in
>> this way, as it makes it relatively easy to identify quickly at a
>> glance the third party libs that are in use in the module.
> 
> Sure, I'm not saying don't group. I'm saying that it's actually 
> impossible to create a formal definition for group 2. So use it as 
> guidelines don't enforce in code.

I'm -1 on manual enforcement of these rules.  I hate -1'ing a patch
(especially one that's been in review for a while) just for import
grouping issues, and if I'm not going to -1 for that then why even
pretend it's something we want to do?

I'd rather have an exception mechanism for the check where we can make
a list of modules that can be in either stdlib or third-party and
allow both.  I don't think the Python stdlib is changing so fast that
we couldn't keep such a list maintained and it would allow these
checks to still be automated.  In fact, it should be possible to
generate the list programmatically.

Obviously I am signing up to do this work if it's something we can
agree on.

> 
>>> We have to remember we're all humans, and it's ok to have grey
>>> space. Like in 305, you *should* group the libraries if you
>>> can, but stuff like that should be labeled as 'nit' in the
>>> review, and only ask the author to respin it if there are other
>>> more serious issues to be handled.
>> 
>> Ya, no real disagreement on that.
>> 
>>> Let's optimize a little more for fun, and stop throwing -1s for
>>> silly things. :)
>> 
>> ++
>> 
>> I would also love to get rid of H404, otherwise known as the dumb
>> rule that says if you have a multiline docstring, that there must
>> be a summary line, then a blank line, then a detailed
>> description. It makes things like this illegal, which, IMHO, is
>> stupid:
>> 
>> def do_something(self, thing): """We do something with the
>> supplied thing, so that something else is also done with this
>> other thing. Make sure the other thing is of type something. """ 
>> pass
>> 
>> Likewise, I'd love to be able to have a newline start the
>> docstring, like so:
>> 
>> def do_something(self, thing): """ We do something with the
>> supplied thing, so that something else is also done with this
>> other thing. Make sure the other thing is of type something. """ 
>> pass
>> 
>> But there's a rule that prevents that as well...
> 
> Yeh, I'd be happy throwing out the docstring rules all together. I
> feel like docstrings being good or bad is pretty orthoginal to
> these rules the way they enforce things.
> 
>> To be clear, I don't think all hacking rules are silly. To the
>> contrary, there are many that are reasonable and useful. However,
>> I'd prefer to focus on things that make the code more readable,
>> not less readable, and rules that enforce Pythonic idioms, not
>> some random hacker's idea of good style.
> 
> Correct, I'm with you. But I also feel like we've wandered across a
> line in the current implementation, and it's time to prune back.
> 
> Because hacking supports local rules, if any particular project
> wants to go overboard in rules, so be it. But lets not do that in
> hacking itself.

I made this comment on one of the reviews too, but it would be nice if
we had a central repository for these sorts of checks.  Otherwise
we're going to end up with any project that wants to enforce, say, the
docstring checks having to rewrite the checks in their local repo and
there will be a bunch of different implementations of the same rule.

> 
> -Sean
> 
> 
> 
> _______________________________________________ OpenStack-dev
> mailing list OpenStack-dev at lists.openstack.org 
> http://lists.openstack.org/cgi-bin/mailman/listinfo/openstack-dev
> 

-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1
Comment: Using GnuPG with Thunderbird - http://www.enigmail.net/

iQEcBAEBAgAGBQJTqDfxAAoJEDehGd0Fy7uqm9UIAIsb8Pk2yyLqGzrUnPRWZNLZ
nlLoKDwMIu6iQmP6CYuGNZtxse4anUZN0Xzm5q/E90S1jyDRP4uHCTrw6xwNaQ69
yauP37Y4pA18rv/zjABpckv/USyL0vantWOXPxnQEkZsRIUF5Lh0zehI9AkIVlwy
PfWudRX87wIfXQTjaLRW4otFt5D3udqIfinLS9LBK+2YlpZZ95htTMLBI0ZMzat8
5qVwPFrZ/tOW7DQmAzisYQ7C46oB9DLB2cehWZgoEk0dkLrBiEPOojRoHpPK3s0J
fIaTe/PLY5t9wSMxxJehLwkpFcu1M2CdLomLtuspXEjPpzmSsnDtSWjeMsgvMjs=
=jLGp
-----END PGP SIGNATURE-----