[Openstack] coding standards (was: review for implement dhcp agent for quantum)
Dan Wendlandt
dan at nicira.com
Wed Jul 4 01:50:14 UTC 2012
I talked with folks a bit about this offlist. Here's the summary:
I think everyone agrees that there is a value in enforcing style guidelines
that go beyond what can be mechanically enforced via pep8, namely, things
covered in HACKING.rst (such as doc-strings formatting). Tools for
automatic checking are ideal, but they don't always exist.
I think everyone (including the reviewer) would agree that the review
comments went beyond this, with spelling and grammer suggestions. My
feeling is that assuming the comment is readable, such spelling/grammer
comments should be viewed as suggestions, not requirements. I think both
reviewer and reviewee agrees with this.
Perhaps part of the problem here is that the review comments about spelling
grammar nits where originally intepreted by the reviewee as being
'required' for approval of the patch. Now that it is clear that they are
not, I believe the concerns are alleviated.
In the future, we should make sure that reveiwers are more clear about what
comments are 'suggestions' vs. 'requirements', particularly when reviewing
code from people new to the project. I don't think anyone is saying that a
reviewer shouldn't be allowed to point out spelling/grammer issue in code
in a friendly way. As I said in the other thread, I personally think its
helpful when a reviewer points out a typo of mine.
Dan
On Wed, Jul 4, 2012 at 1:33 AM, Monty Taylor <mordred at inaugust.com> wrote:
>
>
> 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
> >
>
>
> _______________________________________________
> 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
>
--
~~~~~~~~~~~~~~~~~~~~~~~~~~~
Dan Wendlandt
Nicira, Inc: www.nicira.com
twitter: danwendlandt
~~~~~~~~~~~~~~~~~~~~~~~~~~~
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.openstack.org/pipermail/openstack/attachments/20120704/29d14777/attachment.html>
More information about the Openstack
mailing list