Opened 15 months ago

Closed 9 months ago

#6648 enhancement closed fixed (fixed)

tap.py should conform to docstring and whitespace standards

Reported by: shira Owned by: shira
Priority: normal Milestone:
Component: mail Keywords: documentation
Cc: Branch: branches/tap-doc-6648
(diff, github, buildbot, log)
Author: shira Launchpad Bug:

Description

The docstrings in tap.py 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 15 months ago by shira

  • Author set to shira
  • Branch set to branches/tap-doc-6648

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

comment:2 Changed 15 months ago by shira

(In [39349]) Fixed docstrings and whitespace

refs #6648

comment:3 Changed 15 months ago by shira

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 15 months ago by shira

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

refs #6648

comment:5 Changed 15 months ago by shira

  • Keywords documentation review added
  • Owner shira deleted

comment:6 Changed 15 months ago by shira

(In [39368]) Add topfile.

refs #6648

comment:7 Changed 15 months ago by hynek

  • Keywords review removed
  • Owner set to shira

Thanks!

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 15 months ago by hynek

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 14 months ago by shira

(In [39771]) Revised API documentation

refs #6648

comment:10 Changed 14 months ago by shira

  • Keywords review added
  • Owner shira 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 tap.py. 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 12 months ago by lvh

  • Keywords review removed

The buildbots look clean now: https://buildbot.twistedmatrix.com/boxes-supported?branch=/branches/tap-doc-6648 , and a cursory review appears to look good. Feel free to merge :)

comment:12 Changed 12 months ago by exarkun

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 12 months ago by shira

  • Owner set to shira

comment:14 Changed 11 months ago by shira

  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 9 months ago by shira

(In [41466]) Fix docstring link

refs #6648

comment:16 Changed 9 months ago by shira

  • Keywords review added
  • Owner shira deleted

The issues from comment #12 have been addressed.

comment:17 Changed 9 months ago by hynek

  • 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 9 months ago by shira

  • Owner set to shira

comment:19 Changed 9 months ago by shira

  • Resolution set to fixed
  • Status changed from new to closed

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

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

Note: See TracTickets for help on using tickets.