Opened 8 years ago

Closed 8 years ago

#6653 enhancement closed fixed (fixed)

pop3client 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/pop3client-doc-6653-2
branch-diff, diff-cov, branch-cov, buildbot
Author: shira

Description

The docstrings in pop3client.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.

Attachments (1)

pop3client-doc-6653.patch (5.7 KB) - added by Becca Bainbridge 8 years ago.

Download all attachments as: .zip

Change History (23)

comment:1 Changed 8 years ago by Stacey Sern

Author: shira
Branch: branches/pop3client-doc-6653

(In [39371]) Branching to 'pop3client-doc-6653'

comment:2 Changed 8 years ago by Stacey Sern

(In [39454]) Fix docstrings and whitespace.

refs #6653

comment:3 Changed 8 years ago by Stacey Sern

(In [39455]) Added missing epytext markups.

refs #6653

comment:4 Changed 8 years ago by Stacey Sern

Keywords: review added
Owner: Stacey Sern deleted

comment:5 Changed 8 years ago by Richard Wall

Owner: set to Richard Wall
Status: newassigned

comment:6 Changed 8 years ago by Richard Wall

Keywords: review removed
Owner: changed from Richard Wall to Stacey Sern
Status: assignednew

Thanks Shira,

Notes:

Points:

  1. branches/pop3client-doc-6653/twisted/mail/pop3client.py
    1. I think much of what I said in ticket:6654#comment:6 applies here too (C{} > L{}, succinct @rtypes, L{} for builtin types, L{bytes} instead of L{str}, links directly to init etc.
      1. although I like the way you document the expected parameter types for callable @rtype
    2. "An error indicating": I don't like these additions. The Exception docstrings probably end up being presented to a user as error messages - so its better if they just say eg "Secure authentication was required but no mechanism could be found."
    3. "137 @rtype: (C{int}, C{int}) ": should be L{tuple}.
    4. Acronyms like "UIDL" would probably be more readable if you put them in I{UIDL}.
      1. http://epydoc.sourceforge.net/manual-epytext.html#basic-inline-markup
    5. 290 @type _xform: C{NoneType} or callable": Use L{callable}
      1. And again on following line.
    6. "257 @type _challengeMagicRe: regular expression object ": Link to the appropriate L{re} type.
    7. Use I{} for states eg LONG_INITIAL, WELCOME.
  1. twisted/mail/topfiles/6653.doc
    1. Try and give a more specific description of the API improvements or just make this an empty .misc file.

This isn't as thorough as the last review, sorry. The Sun's come out, the cricket's getting exciting and I've lost concentration (about halfway through the diff.)

Please resubmit after addressing or answering the numbered points above and paste a link to some clean build results.

-RichardW.

comment:7 Changed 8 years ago by Stacey Sern

(In [39774]) Revised API documentation

refs #6653

comment:8 Changed 8 years ago by Stacey Sern

Keywords: review added
Owner: Stacey Sern deleted

Fixed everything mentioned with the following exceptions:

Issue 2: I don't believe that the exception docstrings end up being used for error messages to the user. I rewrote these because I've been trying to consistently describe classes with nouns. Having a statement like "The server sent an extremely long line" doesn't really explain that the class represents an error. It's most apparent on the api page for the module (http://twistedmatrix.com/documents/current/api/twisted.mail.pop3client.html). I just think it's missing something if exception classes don't explain that they represent an error.

Issues 4 and 7: I tried this but in the api documentation, the italics don't stand out any more than the caps do and in the doctstring itself, it is harder to read with the I{} markup.

Issue 5: Although callable does generate a link, it is to a callable function not something that describes the general category of callables. I chose not to markup general categories like callables, iterables and iterators.

Also made a few changes to comments to remove some old twisted checker errors.

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

  • Removing (optional) in param descriptions
  • Including a locally usable name in the link
  • Removing blank lines after class docstrings
  • Changing the format of type descriptions of tuples
  • Changed description of dict which contains two different mappings to remove ambiguity

comment:9 Changed 8 years ago by Stacey Sern

(In [39843]) Add full path in link

refs #6653

comment:10 Changed 8 years ago by Richard Wall

Keywords: review removed
Owner: set to Stacey Sern

Thanks Shira,

Ok. This time I've read it in full and the new documentation looks excellent...it must have taken ages!

Notes:

  • Clean build results
  • Merges cleanly

Points (mostly nit picking):

  1. branches/pop3client-doc-6653/twisted/mail/pop3client.py
    1. Personally, I'm not keen on the (E{1}) syntax for documenting the tuple indexes -- I think it makes the raw docstrings too difficult to read and the E{} is likely to confuse people...it certainly confused me until I looked it up.
      1. I like the 2-L{tuple} syntax though.
    2. nit. *L{NoneType <types.NoneType>}* seems over complicated, I suggest we use L{None} and wait for Pydoctor to be updated.
    3. 593 code, status = _codeStatusSplit(line)
      1. Looks like an unnecessary code change
    4. "Switch to encrypted communication using TLS or SSL. " probably should be "TLS / SSL" or just "TLS"
      1. https://thoughtstreams.io/glyph/there-is-no-ssl/
    5. nit. I'm not keen on the "L{dict} of L{bytes} -> L{list}" syntax either. I find it difficult to read (especially in the rendered API docs)...sorry.
      1. I can't suggest a better alternative though...maybe "L{dict}(L{bytes}=L{list}({bytes}))"...hmm no that's impossible to read.
      2. Maybe it's better to use "@type param: L{dict}" and rely on the "@param" description to explain the expected dictionary format.

Please consider all the points above, but some of them are subjective.

Please merge after addressing at least 1.1, 1.3 and 1.4.

Thanks again.

-RichardW.

Changed 8 years ago by Becca Bainbridge

Attachment: pop3client-doc-6653.patch added

comment:11 Changed 8 years ago by Becca Bainbridge

Keywords: review added

Removed escaping and fixed indexing of tuples.

comment:12 Changed 8 years ago by Glyph

(In [40968]) Committing pop3client-doc-6653.patch for review. refs #6653

comment:13 Changed 8 years ago by Stacey Sern

Keywords: review removed

comment:14 Changed 8 years ago by Stacey Sern

Branch: branches/pop3client-doc-6653branches/pop3client-doc-6653-2

(In [41222]) Branching to 'pop3client-doc-6653-2'

comment:15 Changed 8 years ago by Stacey Sern

(In [41223]) Merge forward and resolve conflict from ticket #6761.

refs #6653

comment:16 Changed 8 years ago by Stacey Sern

(In [41224]) Change markup for additional None references

refs #6653

comment:17 Changed 8 years ago by Stacey Sern

Keywords: review added

Addressed the issues in comment 10. Additionally fixed the markup for regex and fixed two instances of incorrect tuple notation. Changed all references to None (the value as opposed to the type NoneType) to C{None}.

Because of the additional changes, I am putting it back for review. I missed adding the reference to this ticket to the first commit (the second is in comment 16) for these changes. The changes are in changeset http://twistedmatrix.com/trac/changeset/41221/branches/pop3client-doc-6653/twisted/mail/pop3client.py

comment:18 Changed 8 years ago by Stacey Sern

comment:19 Changed 8 years ago by Richard Wall

Owner: changed from Stacey Sern to Richard Wall
Status: newassigned

comment:20 Changed 8 years ago by Richard Wall

Keywords: review removed
Owner: changed from Richard Wall to Stacey Sern
Status: assignednew

Thanks Shira,

I think this is fine. But I've added some suggestions below. Implement them if you feel like it.

Notes:

  1. Merges cleanly
  2. Build Results
    1. Documentation builder seems happy

Points:

  1. source:branches/pop3client-doc-6653-2/twisted/mail/pop3client.py
    1. "72 @type consumer: callable that takes L{object} "
      1. L{object} seems a bit unnecessary. You may as well just say "callable"
      2. Same on lines 299, 415, 1060, 1068, 1093, 1098
    2. "extremely long line"
      1. Can this be a bit more specific, eg "longer than x chars" or "longer than C{limit}".
      2. Same at "523 Drop the connection when a server response is too long. "
      3. Maybe add a reference / link to L{LineOnlyReceiver.MAX_LENGTH} here
    3. "95 setitem" could be L{setitem}
    4. "104 @type L: L{list} of L{object} "
      1. Again, I think L{object} is unnecessary
    5. "134 STAT"
      1. Maybe put these verbs in I{} to highlight them.
      2. Same with LIST, SHORT etc
    6. "156 @rtype: 2-L{tuple} of (0 L{int}, (1) L{int} "
      1. Missing closing bracket ")"
      2. But actually I still think it would be more readable without the indexes
      3. ie "(L{int}, L{int})
      4. ie r41111
    7. "239 be allowed if the server"
      1. Could do with a comma ie "allowed, "
    8. "324 C{None}"
      1. Missing full stop
    9. "421 L{bytes} and returns L{object} "
      1. returns L{object} seems too vague
      2. Same with "takes L{object}" on 428
    10. "430 On an okay response"
      1. Is "okay" a name defined by the protocol? If so, I think it would be better in I{}.
      2. Or maybe it just means "success" of the deferred? In which case, just say "On success"
      3. I think this is repeated elsewhere in the branch.

As I said, those are mostly suggestions. Please merge. If you do make any further changes, it's worth forcing a new documentation build before merging.

comment:21 Changed 8 years ago by Stacey Sern

(In [41227]) Incorporate code review comments

refs #6653

comment:22 Changed 8 years ago by Stacey Sern

Resolution: fixed
Status: newclosed

(In [41228]) Merge pop3client-doc-6653-2: Fix docstrings for pop3client.py

Author: shira Reviewer: rwall Fixes: #6653

Note: See TracTickets for help on using tickets.