Merge lp:~termie/nova/more_docstring into lp:~hudson-openstack/nova/trunk

[termie]

attempts to make the docstring rules clearer

[Todd Willey (xtoddx)]

Should add a comment about leaving a blank line at the bottom of a multiline docstring, so people know it is intentional?

[Jay Pipes (jaypipes)]

Why isn't this style allowable?

"""
This is the first line.

This is some more details that describe
the function/class more...

:param blah: Blah
"""

pep257 specifically mentions the above style is fine:

"The summary line may be used by automatic indexing tools; it is important that it fits on one line and is separated from the rest of the docstring by a blank line. The summary line may be on the same line as the opening quotes or on the next line."

with or without an additional newline at the end (what Todd mentioned...)

-jay

[Brian Lamar (blamar)]

While I think it's pretty easy to require Sphinx-compatible formatting for describing parameters, I think it's crazy to disallow a leading newline in multi-line comments. IMO this:

def an_example_method(bar, baz):
    """
    A multi-line comment.

    :param bar: the bar parameter
    :param baz: the baz parameter
    :returns: foo string
    """
    pass

...is far superior (in terms of readability) to:

def an_example_method(bar, baz):
    """A multi-line comment.

    :param bar: the bar parameter
    :param baz: the baz parameter
    :returns: foo string
    """
    pass

Why not just say we should do PEP-257 + Sphinx-compatible parameter descriptions?

review: Needs Information

[termie (termie)]

> Should add a comment about leaving a blank line at the bottom of a multiline
> docstring, so people know it is intentional?

There is already a comment about it.

[termie (termie)]

A leading newline is not acceptable, the newline is just extra whitespace and changes the appearance between a multi-line and a single-line docstring unnecessarily.

Our style guide is more strict than the PEP-257 just as it is more strict than PEP-8. Having a consistent codebase is worth sacrificing a small amount of style flexibility.

[Jay Pipes (jaypipes)]

> A leading newline is not acceptable, the newline is just extra whitespace and
> changes the appearance between a multi-line and a single-line docstring
> unnecessarily.

I don't agree.

> Our style guide is more strict than the PEP-257 just as it is more strict than
> PEP-8. Having a consistent codebase is worth sacrificing a small amount of
> style flexibility.

Then I vote we remove the reference to pep 257, since we're not following its guidelines.

[Brian Lamar (blamar)]

I would be rather all reviewers call people out on *having* docstrings for all methods rather than calling them out for an extra newline. I'd say disallowing a newline is borderline asinine in that it's not going to be enforceable.

[Vish Ishaya (vishvananda)]

Can we just be consistent? I don't really care either way, but it does look nice if we use the same style for the whole code base.

On Mar 28, 2011, at 11:33 AM, Brian Lamar wrote:

> I would be rather all reviewers call people out on *having* docstrings for all methods rather than calling them out for an extra newline. I'd say disallowing a newline is borderline asinine in that it's not going to be enforceable.
> --
> https://code.launchpad.net/~termie/nova/more_docstring/+merge/55198
> You are requested to review the proposed merge of lp:~termie/nova/more_docstring into lp:nova.

[termie (termie)]

> I would be rather all reviewers call people out on *having* docstrings for all
> methods rather than calling them out for an extra newline. I'd say disallowing
> a newline is borderline asinine in that it's not going to be enforceable.

It is easy to enforce, justinsb already has a branch out that does the checking :)

[justinsb (justin-fathomdb)]

Yes, agreeing the rules is obviously going to be a little controversial but
the most important thing for me is consistency as vish says. It may be we
need the ptl to just make the call for each project. This branch should let
that be enforced painlessly.

I am all for more docstrings, but they can be overkill. I think we should
only require docstrings on public methods, and not in tests. In addition,
does anyone know how to do docstring inheritance? if I'm implementing a
method defined and well documented by a base class, I want to point at those
docs.
On Mar 28, 2011 11:50 AM, "termie" <email address hidden> wrote:
>> I would be rather all reviewers call people out on *having* docstrings
for all
>> methods rather than calling them out for an extra newline. I'd say
disallowing
>> a newline is borderline asinine in that it's not going to be enforceable.
>
> It is easy to enforce, justinsb already has a branch out that does the
checking :)
> --
> https://code.launchpad.net/~termie/nova/more_docstring/+merge/55198
> You are requested to review the proposed merge of
lp:~termie/nova/more_docstring into lp:nova.

[termie (termie)]

> I am all for more docstrings, but they can be overkill. I think we should
> only require docstrings on public methods, and not in tests. In addition,
> does anyone know how to do docstring inheritance? if I'm implementing a
> method defined and well documented by a base class, I want to point at those
> docs.

It looks like sphinx supports some cross-referencing: http://sphinx.pocoo.org/domains.html#python-roles

It isn't quite inheritance but at least gets people to the right place if you think they will have a hard time.

> On Mar 28, 2011 11:50 AM, "termie" <email address hidden> wrote:
> >> I would be rather all reviewers call people out on *having* docstrings
> for all
> >> methods rather than calling them out for an extra newline. I'd say
> disallowing
> >> a newline is borderline asinine in that it's not going to be enforceable.
> >
> > It is easy to enforce, justinsb already has a branch out that does the
> checking :)
> > --
> > https://code.launchpad.net/~termie/nova/more_docstring/+merge/55198
> > You are requested to review the proposed merge of
> lp:~termie/nova/more_docstring into lp:nova.

[Brian Lamar (blamar)]

@termie

Can I get some of the background or your thought process as to why a leading newline is "not acceptable"?

[justinsb (justin-fathomdb)]

I'm fine with the rules as they stand: it's more important that we have a set of rules to follow than that we think the rules are perfect. These are the rules we've always had, so let's just start sticking with them. We can discuss changing our rules at the design summit I guess.

termie: I think your edit may have lost the "After that an extra newline then close the quotations." i.e. a blank line before the closing """ I have rule for that one in termiebot now.

Otherwise, lgtm

Brian: Checking that docstrings are present in my other branch looks to be a little hard at the moment, because I haven't got inspection to work reliably (it blows up when checking itself, for example, because of a duplicate flag) PyLint can do it, but can't ignore private methods.

review: Approve

[Jay Pipes (jaypipes)]

On Mon, Mar 28, 2011 at 8:20 PM, justinsb <email address hidden> wrote:
> We can discuss changing our rules at the design summit I guess.

Yes, please.

[lp:~termie/nova/more_docstring]

899. By termie on 2011-03-29

accidentally dropped a sentence

[termie (termie)]

jay: while I am fine discussing the rules at the summit, it is probably a good way to actually disseminate the knowledge, keep in mind that these are style rules that have been here since the beginning of the project.

[termie (termie)]

justinsb: added that line back, i thought the next paragraph was covering it but that is about the newline _after_ the quotations in class docstrings

[Jay Pipes (jaypipes)]

@termie: you just recently added the "rule" on the docstring style to HACKING, so don't give me that crap about these "rules" being here since the beginning of the project. They might have been in your head, but they weren't written down and certainly haven't been enforced during code review.

jpipes@serialcoder:~/repos/nova/trunk$ bzr blame --verbose HACKING | grep -n3 Docstring
48- | from nova.endpoint import api
49- | from nova.endpoint import cloud
50-641.3.1 github@ |
51: | Docstrings
52- | ----------
53- | """Summary of the function, class or method, less than 80 characters.
54- |
jpipes@serialcoder:~/repos/nova/trunk$ bzr log --long -r641.3.1
------------------------------------------------------------
revno: 641.3.1
committer: termie <email address hidden>
branch nick: hacking_update
timestamp: Wed 2011-02-02 13:13:10 -0800
message:
  some updates to HACKING to describe the docstrings

[termie (termie)]

jaypipes: slow. your. roll.

As is obvious from the state of the codebase, nobody has been enforcing style rules.

The examples were added to HACKING to make it easier to point at something rather than having to give a full example every time.

[Todd Willey (xtoddx)]

lgtm

review: Approve

[Sandy Walsh (sandy-walsh)]

Will there be a blanket search-n-replace if there is a style change or will we have to live with the broken windows?

I don't like the "let's pick away at the change" approach. Never gets done completely. If this gets approved, there should be a bug filed to one-shot change all the source.

review: Needs Information

[Brian Lamar (blamar)]

review: Abstain

[termie (termie)]

I've been going through and updating the source, I've got about 9 branches open so far for the changes. I think the important part is to start gating the merge proposals more strictly so that we stop introducing new issues.

[termie (termie)]

(Also, Sandy, this isn't a style change, it is just an update to explain it more clearly so that there is less confusion. And I do think there is a docstring cleanup blueprint, sort of: https://blueprints.launchpad.net/nova/+spec/docstrings-and-comments)

[Christopher MacGown (0x44)]

lgtm

review: Approve

[termie (termie)]

looks like this has more than enough approves and sandy hasn't responded in a couple weeks, approving it

=== modified file 'HACKING'
--- HACKING	2011-02-12 21:30:31 +0000
+++ HACKING	2011-03-29 18:17:32 +0000
@@ -50,17 +50,24 @@
 
 Docstrings
 ----------
-  """Summary of the function, class or method, less than 80 characters.
-
-  New paragraph after newline that explains in more detail any general
-  information about the function, class or method. After this, if defining
-  parameters and return types use the Sphinx format. After that an extra
-  newline then close the quotations.
+  """A one line docstring looks like this and ends in a period."""
+
+
+  """A multiline docstring has a one-line summary, less than 80 characters.
+
+  Then a new paragraph after a newline that explains in more detail any
+  general information about the function, class or method. Example usages
+  are also great to have here if it is a complex class for function. After
+  you have finished your descriptions add an extra newline and close the
+  quotations.
 
   When writing the docstring for a class, an extra line should be placed
   after the closing quotations. For more in-depth explanations for these
   decisions see http://www.python.org/dev/peps/pep-0257/
 
+  If you are going to describe parameters and return values, use Sphinx, the
+  appropriate syntax is as follows.
+
   :param foo: the foo parameter
   :param bar: the bar parameter
   :returns: description of the return value