Opened 4 years ago

Closed 3 years ago

#6637 enhancement closed fixed (fixed)

alias.py 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/alias-doc-6637
branch-diff, diff-cov, branch-cov, buildbot
Author: shira

Description

The docstrings in alias.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 (31)

comment:1 Changed 4 years ago by Stacey Sern

Branch: branches/alias-doc-6637

(In [39272]) Branching to 'alias-doc-6637'

comment:2 Changed 4 years ago by Stacey Sern

(In [39273]) Fixed docstrings and whitespace

refs #6637

comment:3 Changed 4 years ago by Stacey Sern

(In [39274]) Fixed problems flagged by TwistedChecker

refs #6637

comment:4 Changed 4 years ago by Stacey Sern

Keywords: review added
Owner: Stacey Sern deleted

comment:5 Changed 4 years ago by Stacey Sern

(In [39277]) Fixed ambiguous type descriptions

refs #6637

comment:6 Changed 4 years ago by Stacey Sern

(In [39278]) Removed trailing white space

refs #6637

comment:7 Changed 4 years ago by Stacey Sern

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

refs #6637

comment:8 Changed 4 years ago by Stacey Sern

(In [39366]) Moved 6637.doc to proper topfiles directory.

refs #6637

comment:9 Changed 4 years ago by Stacey Sern

(In [39367]) Removed old 6637.doc from wrong directory.

refs #6637

comment:10 Changed 4 years ago by Thijs Triemstra

Keywords: review removed
Owner: set to Stacey Sern

Thanks.

  1. | on line 66 should be C{|}
  2. same for : on line 69, should be C{:}
  3. same for # on line 80
  4. fp on line 87 should be C{fp}
  5. filename on line 91 should be C{filename}
  6. memo on line 180 should be C{memo}
  7. line 246 exceeds 80 chars
  8. the indentation for the MessageWrapper.__init__ docstring is wrong
  9. line 514 is missing a period at the end
  10. deferredList on line 666 should be L{DeferredList}
  11. memo on line 786 should be C{memo}
  12. There are a couple of todo's in the module docstring. Can you find the associated tickets (or open tickets if they don't exist) and add a reference to the ticket for each todo in the docstring?

comment:11 Changed 4 years ago by Stacey Sern

(In [39766]) Revised API documentation

refs #6637

comment:12 Changed 4 years ago by Stacey Sern

(In [39769]) Trying something else to fix the twistedchecker error

refs #6637

comment:13 Changed 4 years ago by Stacey Sern

(In [39770]) Going back to original code after discovering twistedchecker problem

refs #6637

comment:14 Changed 4 years ago by Stacey Sern

Keywords: review added
Owner: Stacey Sern deleted

Items 1-11 were fixed. For item 12, the todos were removed. File monitoring is part of Twisted and tickets #6691 and #6692 were created to track the other two issues.

Also, a docstring for AddressAlias.resolve was added, the previous description of aliases beginning with a ":" was corrected and a description of aliases beginning with a "/" was added.

Additional changes were made to incorporate feedback from other documentation reviews and for general improvement in readability. They include: Changing type references from str to bytes Using L{} instead of C{} for standard library types Including a locally usable name in L{} Removing blank lines after class docstrings Removing (optional) in param descriptions. Changing the format of type descriptions of tuples Clarifying that args are parameters for the init function, not the base class itself Changing the top file message

This builds with a twistedchecker error due to the "}" after the "#" on line 78. This is due to a bug in twistedchecker (bug report #1217144).

comment:15 Changed 4 years ago by Stacey Sern

Here's the list of additional changes from comment #14 in list form:

  1. Changing type references from str to bytes
  2. Using L{} instead of C{} for standard library types Including a locally usable name in L{}
  3. Removing blank lines after class docstrings
  4. Removing (optional) in param descriptions
  5. Changing the format of type descriptions of tuples
  6. Clarifying that args are parameters for the init function, not the base class itself
  7. Changing the top file message

comment:16 Changed 4 years ago by Stacey Sern

(In [39803]) Revised type description

refs #6637

comment:17 Changed 4 years ago by Stacey Sern

(In [39804]) Revised type description

refs #6637

comment:18 Changed 4 years ago by Stacey Sern

(In [39806]) Use argument instead of parameter in documentation

refs #6637

comment:19 Changed 4 years ago by Stacey Sern

(In [39921]) Remove docstrings for IAlias

refs #6637

comment:20 Changed 4 years ago by Stacey Sern

The docstrings for IAlias have been removed because they are being included in ticket #6638.

comment:21 Changed 4 years ago by Richard Wall

Owner: set to Richard Wall
Status: newassigned

comment:22 Changed 4 years ago by Richard Wall

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

Thanks shira,

The new documentation looks great (as usual) and most of my points are nit picks.

Notes:

Points:

  1. source:branches/alias-doc-6637/twisted/mail/alias.py
    1. Worth creating tickets for the TODO items that were deleted? Or have they actually been done?
    2. handle
      1. "L{dict} of L{bytes} -> L{list} of L{bytes}" I think I've said it in previous reviews, but I find this hard to read. Personally I think it would be clearer to just say L{dict} and describe the format in the @param.
        1. What style have you settled on in the other mail documentation branches?
        2. Maybe we can discuss it in #twisted-dev and decide once and for all what the house style should be.
      2. It looks to me like filename is actually "The path to the aliases file".
    3. loadAliasFile
      1. I don't like L{NoneType <types.NoneType>}. Lets just ask mwh to fix pydoctor so it properly renders L{None}.
      2. Again, I think filename should be documented as the "path to the aliases file".
    4. AliasBase
      1. "ivar original" looks like it could be a link to the constructor (like you've done for domains)
    5. AddressAlias
      1. "ivar alias" can that be a link to the constructor? Maybe not since it gets wrapped in smtp.Address
    6. FileWrapper
      1. Are these filenames or file paths?
      2. "file name *in* which the message *is* stored" might be better.
    7. FileAlias
      1. "@type args" this seems unnecessary since you're linking to AliasBase which has the type documentation already.
    8. MessageWrapper
      1. "ivar reactor" can be a link to the constructor.
    9. connectionLost
      1. You could remove the "pass" statement now that there's a docstring.
    10. ProcessAliasProtocol
      1. "A process protocol which calls an errback" I'm not sure about that. I think I prefer the original description if you replace "callback" with "errback".

I think it would be worth discussing the documentation of variable types in #twisted-dev (maybe that discussion has even taken place already). In any case, I've been using a different style in my branches and I'm probably inconsistent from one branch to another. So it would be nice to document the house style and then stick to it.

I'm happy for this branch to be merged if that discussion takes place and the documentation in this branch is updated to reflect that style. And you address or answer the other numbered points above.

Thanks again.

-RichardW.

comment:23 Changed 4 years ago by Stacey Sern

Thanks for all the reviews. Sorry I've been a bit slow responding. All of the mail documentation uses the same style. I'm fine changing the style for dict to remove the arrow. This was also brought up in ticket #6717 and I just responded there about a new format.

Regarding L{NoneType <types.NoneType>}, I also don't like it. It would be nice if pydoctor knew how to resolve L{NoneType}. None is a constant value in Python, not a type so I don't think L{None} would be technically correct. I will put in a ticket for pydoctor to resolve L{NoneType}. After that is done, I will change the L{NoneType <types.NoneType>} references.

comment:24 Changed 4 years ago by Stacey Sern

The pydoctor ticket for NoneType is https://bugs.launchpad.net/pydoctor/+bug/1252762

comment:25 Changed 3 years ago by Stacey Sern

(In [41440]) Incorporated code review comments and added a few docstrings

refs #6637

comment:26 Changed 3 years ago by Stacey Sern

Keywords: review added

In response to the points in comment #22,

  1. For the removed TODOS, monitoring files for reparsing has been implemented. Ticket #6692 is for non-local alias targets and ticket #6691 is for maildir alias targets.
  2. Changed dictionary notation to L{dict} mapping type1 to type2 consistent with a change throughout the mail docstrings. Changed the description of filename to "full or relative path to the aliases file".
  3. See comment #23. Changed filename as described above.
  4. The reason that original is listed separately as a parameter to init and as an instance variable is that their types are different. The parameter to init is bytes but it is stored as an instance variable as an Address.
  5. The types of the init parameter and the instance variable are different.
  6. These are filenames. Made the change from "to" to "in".
  7. This makes it clear that the arguments need to be passed as a tuple.
  8. The difference is that reactor is an optional parameter to init so it can come in as None. But if so, a reactor is created so the instance variable is never None.
  9. Done
  10. Modified closer to the original wording.

Also added a docstring for IAlias and its function createMessageReceiver and a couple of missing periods. Because of these new docstrings and some of the wording changes, I am putting this back into review.

This builds with two twistedchecker error both of which are due to bugs in twistedchecker: https://bugs.launchpad.net/twistedchecker/+bug/1217144 and https://bugs.launchpad.net/twistedchecker/+bug/1217379.

comment:27 Changed 3 years ago by Stacey Sern

Owner: Stacey Sern deleted

comment:28 Changed 3 years ago by Richard Wall

Owner: set to Richard Wall
Status: newassigned

comment:29 Changed 3 years ago by Richard Wall

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

Thanks shira,

And thanks for raising that pydoctor ticket.

Notes:

  1. All the changes since the last review look sound.
  2. I Built and re-read the API docs. All ok except points below.
  3. Branch merges cleanly.

Points:

  1. ProcessAlias
    1. It looked like these two descriptions might be the wrong way round
      1. "path: The arguments..."
      2. "program: The path..."
      3. Same applies to the spawnProcess arguments

Otherwise it looks great.

Please merge after fixing those descriptions (if you agree).

-RichardW.

comment:30 Changed 3 years ago by Stacey Sern

I reviewed the docstrings for ProcessAlias and think they are correct. I did make one minor edit.

comment:31 Changed 3 years ago by Stacey Sern

Resolution: fixed
Status: newclosed

(In [41590]) Merge alias-doc-6637: Fix docstrings and whitespace in alias.py

Author: shira Reviewer: thijs, rwall Fixes: #6637

Note: See TracTickets for help on using tickets.