Open Stack

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.

> 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
> ~~~~~~~~~~~~~~~~~~~~~~~~~~~
>