Opened 7 years ago

Closed 7 years ago

#6648 enhancement closed fixed (fixed) should conform to docstring and whitespace standards

Reported by: Stacey Sern Owned by: Stacey Sern
Priority: normal Milestone:
Component: mail Keywords: documentation
Cc: Branch: branches/tap-doc-6648
branch-diff, diff-cov, branch-cov, buildbot
Author: shira


The docstrings in should be updated to provide descriptions of all classes, functions and methods as well as their parameters and attributes. The spacing before classes and functions should conform to the coding standards.

Change History (19)

comment:1 Changed 7 years ago by Stacey Sern

Author: shira
Branch: branches/tap-doc-6648

(In [39348]) Branching to 'tap-doc-6648'

comment:2 Changed 7 years ago by Stacey Sern

(In [39349]) Fixed docstrings and whitespace

refs #6648

comment:3 Changed 7 years ago by Stacey Sern

The functions name opt_XXX do not meet the strict docstring requirements because they double as help strings for twistd mail. Also, three blank lines contain whitespace. This was necessary so that the docstrings are formatted properly for both the api documentation and the help.

comment:4 Changed 7 years ago by Stacey Sern

(In [39354]) Change mentions of interface to interface provider.

refs #6648

comment:5 Changed 7 years ago by Stacey Sern

Keywords: documentation review added
Owner: Stacey Sern deleted

comment:6 Changed 7 years ago by Stacey Sern

(In [39368]) Add topfile.

refs #6648

comment:7 Changed 7 years ago by Hynek Schlawack

Keywords: review removed
Owner: set to Stacey Sern


Let’s see how to make this even better:

  1. General comments
    1. I understand that L{} should be used for types from the standard library, see also #6318
    2. Empty lines after a docstring like line 66 should be avoided.
    3. The oxford comma should be used in enumerations because it’s more clear.
    4. (optional) is redundant if the argument has a default value. A grep on Twisted’s code base indicates that it’s generally not used in docstrings.
  2. As for Options, I don’t think we should document fields like optParameters – or document Options at all for that matter except for a short explanation.
    1. Also I don’t think _protoDefaults should be documented in a docstring. I believe private fields should be documented using a normal comment otherwise they become public API.
  3. Line 166 is funky, probably an accidental line merge.
  4. There’s only one empty line between opt_user and opt_bounce_to_postmaster.
  5. I know this is borderline OCD but I believe it should be i.e. (two points).

Thanks again, please address the issues and re-submit for review!

comment:8 Changed 7 years ago by Hynek Schlawack

Two more things:

  1. As jp just pointed out on IRC: there is no such thing as str anymore – only bytes and unicode – please fix those too.
  2. Additionally, twistedchecker has a few relevant complaints:
W9202:163,4:Options.opt_pop3: Missing epytext markup @param for argument "description"
W9011:165,0: Blank line contains whitespace
W9202:175,4:Options.opt_smtp: Missing epytext markup @param for argument "description"
C0301:194,0: Line too long (88/79)
W9202:199,4:Options.opt_maildirdbmdomain: Missing epytext markup @param for argument "domain"
C0301:210,0: Line too long (100/79)
C0301:212,0: Line too long (91/79)
C0103:216,23:Options.opt_user: Invalid name "user_pass" (should match ([a-z_][a-zA-Z0-9]*)$)
W9202:217,4:Options.opt_user: Missing epytext markup @param for argument "user_pass"
C0301:223,0: Line too long (92/79)
W9202:239,4:Options.opt_aliases: Missing epytext markup @param for argument "filename"
C0301:257,0: Line too long (80/79)
W9501:317,39:Options.postOptions: String formatting operations should always use a tuplefor non-mapping values
C0301:333,0: Line too long (84/79)

comment:9 Changed 7 years ago by Stacey Sern

(In [39771]) Revised API documentation

refs #6648

comment:10 Changed 7 years ago by Stacey Sern

Keywords: review added
Owner: Stacey Sern deleted

Fixed comment 7 issues 1, 3, 4 and 5 and comment 8 issue 1.

Regarding comment 7 issue 2, the coding standards state that every method, function, class and module should have docstrings. The case for Options being documented is that if someone wanted to understand or change the options for the mail module they should be able to do it looking only at They shouldn't have to go to usage.options to figure out how it works.

That being said, opt_pop3, opt_smtp, opt_maildirdbmdomain, opt_user and opt_aliases are exceptions to everything being fully documented because their docstrings are used directly in the help message for twistd mail. That is why they generate missing epytext markup errors from twisted checker that can't be corrected (Comment 8 issue 2, lines 163, 175, 199, 217 and 239).

In terms of documenting private variables, there are many instances of private variables which are documented (in classes twisted.web.wsgi.WSGIResource and twisted.protocols.tls.TLSMemoryBIOProtocol for example). All private functions are included in the API even if they don't have a docstring. Private methods and functions are shaded gray. So I left in the documentation for private instance variables.

Additional changes were made to incorporate feedback from other documentation reviews and for general improvement in readability. They include:

  1. Including a locally usable name in the link
  2. Changing the format of type descriptions of tuples and fixed-length lists
  3. Changing the top file message

comment:11 Changed 7 years ago by lvh

Keywords: review removed

The buildbots look clean now: , and a cursory review appears to look good. Feel free to merge :)

comment:12 Changed 7 years ago by Jean-Paul Calderone

Thanks for your work on this ticket shira, lvh.

Some of the new additions are documentation for things like optParameters, optFlags, and other attributes the meaning of which is defined by twisted.python.usage.Options and which these subclasses are just providing values for.

It seems like it may not make a great deal of sense to re-document these attributes on every single Options subclass. Instead, the documentation for them on Options should be made as good as possible and other code should probably refer to that documentation.

Beyond that:

  1. Python sequences are zero indexed. I am guessing that the (E{(N)})) markup is indicating tuple indexes. This notation is a bit confusing to me - largely because it is unique (as far as I can tell) and unexplained. Difficulty with documenting long tuples is another good reason not to use long tuples. Perhaps you could file a ticket for replacing this with a named type with attributes that can be documented.
  2. Types that are bytes should have example values specified as C{b"foo"} not C{"foo"}.
  3. -> isn't Python syntax or a particularly widely used convention. I suggest L{dict} mapping L{foo} to L{bar}. Or L{dict} with keys ... and values ....

Thanks again.

comment:13 Changed 7 years ago by Stacey Sern

Owner: set to Stacey Sern

comment:14 Changed 7 years ago by Stacey Sern

  1. I will change the tuple index number to be zero based in all the mail documentation. I came up with the funny markup to get around some problem with generating the documentation but when I tried to figure out what the problem was, I found that it works without escaping the number so I will change that as well.
  1. I will change the dictionary format to use the word mapping. Also discussed in ticket #6717.

comment:15 Changed 7 years ago by Stacey Sern

(In [41466]) Fix docstring link

refs #6648

comment:16 Changed 7 years ago by Stacey Sern

Keywords: review added
Owner: Stacey Sern deleted

The issues from comment #12 have been addressed.

comment:17 Changed 7 years ago by Hynek Schlawack

Keywords: review removed

Thanks for your continued important work on making Twisted’s documentation better!

  1. I’ve forced a build and the relevant bots are green.
  2. I would suggest to wrap the examples defaults of opt_* into C{}.
  3. Same for 'NAME=PATH'
  4. I’d prefer C{--no-*} over I{--no-*}
  5. 277: providers, no?

2–4 are up to your aesthetic feeling, 5 to your native speaker feeling. :)

Since everything else has been addressed, feel free to merge when you feel ready.

comment:18 Changed 7 years ago by Stacey Sern

Owner: set to Stacey Sern

comment:19 Changed 7 years ago by Stacey Sern

Resolution: fixed
Status: newclosed

(In [41591]) Merge 6648: Fix docstrings and whitespace in

Author: shira Reviewer: hynek, exarkun Fixes: #6648

Note: See TracTickets for help on using tickets.