Open Stack

On 07/03/2012 05:07 PM, Duncan McGreggor wrote:
> On Tue, Jul 3, 2012 at 5:39 PM, Dan Wendlandt <dan at nicira.com> wrote:
>> Lately, Quantum reviewers have been doing their best to enforce python style
>> guidelines above and beyond the programmatically enforced pep8 checks.  This
>> has happened for many recent reviews, so Mark isn't being singled out here,
> 
> My objection isn't to Mark being singled-out -- my objection is to
> *anyone* engaging in this level of nit-pickery. This is death to
> projects.
> 
> This is coming from a guy who's incredibly anal about his code and
> coding standards, too. I've been coding in Python for over a decade,
> adhering to PEP8 for a considerable period of that time, am a member
> of the notoriously picky Twisted project, and even I was surprised by
> the flood of review comments -- a high number of which contributed
> nothing to the improved readability, maintaiability, or functionality
> of this code under review.
> 
> There were definitely some good points/comments. But there was a lot
> in there that you had to wade through the rest, before you saw them.

I actually am going to need to side with Duncan here, although also I'm
going to slightly disagree- but hopefully we're all used to that by now.

Duncan is right - nitpickery can be quite deadly, but I think what's
worse is when it's vague, not codified, and not checkable.

With pep8, there is a clear document, and there is a tool that a dev can
use to simply check his code. It's not like pylint, where it's literally
impossible to write code which satisfies all of the warnings - it is
completely possible to write code which is pep8 clean (as we all know,
since we are all required to do so)

But the best part about having a tool (other than my single-minded
devotion to automated gating) isn't that we can use it to gate - it's
that a dev can use it locally to verify things before sending them in
for review... and that's great. The death cycle is really about the lag
time. If you write some stuff, then run pep8 - or even nova's hacking.py
- and it tells you things like "Hey Duncan, I don't like it when you
write methods that have the word "is" in the name" - you may think it's
ridiculous, but the feedback cycle is quick and deterministic and it's
not nearly as frustrating.

I think this is why the extra pedanticness in nova has worked out ok
without killing people. The rules are in HACKING and are clear, but
they're also in tools/hacking.py - and we use them as part of the pep8
gate. Because the code is clean to begin with, they're not very onerous
to deal with... they're also simple and deterministic enough, because
someone had to code a flipping check for them.

Once there is a predictable and quick feedback cycle that can be locally
tested, a developer can train himself to write the code that way in the
first place - and they also don't feel like they're being picked on.

SO - I'd recommend a middle ground here - if you want to add additional
strictness in style checking, do what nova did with hacking.py ... we'll
happily add it to the gate if you like. However... just remember that
we're not here to write python style guidelines, or to write python
programs enforcing those guidelines (not even those of us on the CI
team) ... so if you find yourself spending weeks on a new version of
hacking.py, something has probably gone wrong.

>> though admittedly there's a lot of code previously accepted to the codebase
>> that wasn't held to such a high bar.  This attention to style guidelines is
>> generally a good thing,
> 
> I *strongly* disagree.
> 
> /Attention/ to style guidelines is a huge boon to open source
> projects. But /this/ attention seems beyond the pale, like a good idea
> was taken too far and the intent of the guidelines has been lost.
> 
>> though I understand that it can be frustrating,
>> especially for new developers unfamiliar with the rules (I personally like
>> garyk's comment about how he felt dealing with PEP-257, see:
>> http://www.youtube.com/watch?v=lYU-SeVofHs)
> 
> But that's just it: I'm *not* a new developer! I'm a seasoned Python
> hacker, PSF member, obsessive-compulsive neat-freak with code. etc.,
> etc. I haven't ever seen this level of zealous syntax pursuit in any
> well-functioning open source project.
> 
>> As long as reviewer comments are inline with items covered in
>> https://github.com/openstack/quantum/blob/master/HACKING.rst,
> 
> I may have missed something, but a lot of the comments I saw did not
> reference something particular in the HACKING file, nor were many of
> these marked as CONSIDER ...
> 
>> then I
>> consider them fair game for reviews.  If they go beyond that, they should be
>> generally be expressed as a "CONSIDER".
>>
>> If we're unhappy with what is or is not enforced,
> 
> I'm definitely unhappy with what is being enforced and how.
> 
> But even more: if reviews devolve to this level of non-code minutiae,
> how long do you think you will have the hearts and minds of
> enthusiastic contributing coders?
> 
> What about sponsoring organizations? If the review process consumes
> multiple days -- not due to anything functional or checkable, but
> rather somewhat arbitrary linguistic preferences -- and prevents
> contributors from actually getting their *day* jobs done, don't you
> imagine loss of inertia?
> 
> This is the sort of thing that encourages private forks and community
> abandonment. It might be worth reviewing the comments over the last
> few days -- in detail -- and doing so in that light ...
> 
>> we should have a
>> discussion on the ML and update HACKING.rst correspondingly.
> 
>> Sound reasonable?
> 
> It does indeed.
> 
> d
> 
>> Dan
>>
>>
>> On Tue, Jul 3, 2012 at 10:08 PM, Duncan McGreggor <duncan at dreamhost.com>
>> wrote:
>>>
>>> Honestly?
>>>
>>> This seems a bit overboard to me, Maru. Mark's code is passing pep8 for
>>> me.
>>>
>>> That should be enough.
>>>
>>> d
>>>
>>> On Tue, Jul 3, 2012 at 4:49 PM, Maru Newby (Code Review)
>>> <review at openstack.org> wrote:
>>>> Maru Newby has posted comments on this change.
>>>>
>>>> Change subject: implement dhcp agent for quantum
>>>> ......................................................................
>>>>
>>>>
>>>> Patch Set 4: I would prefer that you didn't merge this
>>>>
>>>> (33 inline comments)
>>>>
>>>> Nice cleanup.  As per my last review, minor docstring issues remain.
>>>> assert_bridge_exists also requires attention.
>>>>
>>>> ....................................................
>>>> File quantum/agent/dhcp_agent.py
>>>> Line 65:         """The DhcpAgent daemon runloop."""
>>>> Unnecessary docstring
>>>>
>>>> Line 81:         """This method polls the Quantum database and returns a
>>>> represenation
>>>> PEP257 - prescribe rather than describe
>>>>
>>>> Line 122:         """Returns a dict containing the sets of networks that
>>>> are new,
>>>> PEP257
>>>>
>>>> Line 134:         # We'll first get the networks that have subnets added
>>>> or deleted.
>>>> Avoid use of personal pronouns in docstrings:
>>>>
>>>> We'll first get => Get
>>>>
>>>> Line 142:         # Now update with the networks that have had
>>>> allocations added/deleted.
>>>> Now update => Update
>>>>
>>>> Line 143:         # change candidates are the net_id portion of the
>>>> symmetric diff
>>>> change => Change
>>>>
>>>> Line 158:         """This method will invoke an action on a DHCP driver
>>>> instance."""
>>>> PEP257
>>>>
>>>> Line 177:             # We need to manipulate the state so the action
>>>> will be
>>>> We need to manipulate => Manipulate
>>>>
>>>> Line 180:                 # adding to prev state means we'll try to
>>>> delete it next time
>>>> => Indicate that removal is required
>>>>
>>>> Line 183:                 # removing means it will look like new next
>>>> time
>>>> => Indicate that addition is required
>>>>
>>>> Line 204:             LOG.error(_('You must specify an interface
>>>> driver'))
>>>> Should an error be raised here, or is it desirable for the subsequent
>>>> line to fail?
>>>>
>>>> Line 253:     """A wrapper that augments Sqlsoup results so that they
>>>> look like the
>>>> HACKING - multiline docstring formatting
>>>>
>>>> Line 255:     MAPPING = {
>>>> Blank line required before this one
>>>>
>>>> ....................................................
>>>> File quantum/agent/linux/dhcp.py
>>>> Line 72:         """Enables DHCP for this network."""
>>>> PEP257
>>>>
>>>> Line 79:         """A convenince method to restart the dhcp service for
>>>> the network."""
>>>> PEP257
>>>>
>>>> Line 96:         """Enables DHCP for this network by spawning a local
>>>> process."""
>>>> PEP257
>>>>
>>>> Line 108:         """Enables DHCP for this network by spawning a local
>>>> process."""
>>>> PEP257
>>>>
>>>> Line 145:         return None
>>>> Remove this line
>>>>
>>>> Line 163:         """A convenient way to get the interface name."""
>>>> Unnecessary docstring
>>>>
>>>> Line 165:             # try reading it from the file
>>>> Unnecessary comment
>>>>
>>>> Line 185:         """Spawns a Dnsmasq process for the network."""
>>>> PEP257
>>>>
>>>> Line 215:                        netaddr.IPNetwork(subnet.cidr).network,
>>>> PEP8 vertical alignment (following 2 lines as well)
>>>>
>>>> Line 227:         """Rebuilds the dnsmasq config and signal the dnsmasq
>>>> to reload."""
>>>> PEP257
>>>>
>>>> Line 233:         """ Writes a dnsmasq compatible hosts file."""
>>>> PEP257+remove leading space
>>>>
>>>> Line 257:                                'router',
>>>> PEP8 vertical alignment
>>>>
>>>> Line 266:     """Replaces the contents of file_name with data in a safe
>>>> manner.
>>>> PEP257
>>>>
>>>> Line 268:     We first write to a temp file and then rename. Since POSIX
>>>> renames are
>>>> We first write => Write
>>>>
>>>> Line 269:     atomic, file is unlikely to be corrupted by competing
>>>> writes.
>>>> file => the file
>>>>
>>>> Line 271:     We create the tempfile on the same device to ensure that
>>>> it can be renamed.
>>>> We create => Create
>>>>
>>>> ....................................................
>>>> File quantum/agent/linux/interface.py
>>>> Line 59:     def assert_bridge_exists(self, bridge):
>>>> Assert is a test-centric name. Consider using 'check' instead, which is
>>>> the usual naming convention for methods that raise an exception if a
>>>> condition cannot be satisfied.
>>>>
>>>> Line 60:         """Asserts whether a bridge exists."""
>>>> PEP257
>>>>
>>>> Line 65:             raise AssertionError(msg % args)
>>>> Prefer specific error types to a generic type such as AssertionError.
>>>> The use of assert in testing will raise AssertionError on failure and can be
>>>> confusing if the code under test also relies upon that error type.
>>>>
>>>> Line 85:     """Driver for creating an OVS interface."""
>>>> Blank line required below this one
>>>>
>>>> --
>>>> To view, visit https://review.openstack.org/9064
>>>> To unsubscribe, visit https://review.openstack.org/settings
>>>>
>>>> Gerrit-MessageType: comment
>>>> Gerrit-Change-Id: If3c62965550dc0b0a7982b01d3468e2e07e2b775
>>>> Gerrit-PatchSet: 4
>>>> Gerrit-Project: openstack/quantum
>>>> Gerrit-Branch: master
>>>> Gerrit-Owner: markmcclain <mark.mcclain at dreamhost.com>
>>>> Gerrit-Reviewer: Doug Hellmann <doug.hellmann at dreamhost.com>
>>>> Gerrit-Reviewer: Duncan McGreggor <duncan at dreamhost.com>
>>>> Gerrit-Reviewer: Jenkins
>>>> Gerrit-Reviewer: Juliano G Martinez <juliano.martinez at locaweb.com.br>
>>>> Gerrit-Reviewer: Maru Newby <mnewby at internap.com>
>>>> Gerrit-Reviewer: Salvatore Orlando <salv.orlando at gmail.com>
>>>> Gerrit-Reviewer: Sumit Naiksatam <snaiksat at cisco.com>
>>>> Gerrit-Reviewer: Thiago Morello <morellon at gmail.com>
>>>> Gerrit-Reviewer: Willian Molinari <pothix at pothix.com>
>>>> Gerrit-Reviewer: dan wendlandt <dan at nicira.com>
>>>> Gerrit-Reviewer: garyk <gkotton at redhat.com>
>>>> Gerrit-Reviewer: markmcclain <mark.mcclain at dreamhost.com>
>>
>>
>>
>>
>> --
>> ~~~~~~~~~~~~~~~~~~~~~~~~~~~
>> Dan Wendlandt
>> Nicira, Inc: www.nicira.com
>> twitter: danwendlandt
>> ~~~~~~~~~~~~~~~~~~~~~~~~~~~
>>
> 
> _______________________________________________
> Mailing list: https://launchpad.net/~openstack
> Post to     : openstack at lists.launchpad.net
> Unsubscribe : https://launchpad.net/~openstack
> More help   : https://help.launchpad.net/ListHelp
>