Opened 8 years ago

Closed 3 years ago

#2458 enhancement closed fixed (fixed)

improve twisted.words.im.basechat docstrings

Reported by: exarkun Owned by: jesstess
Priority: normal Milestone:
Component: words Keywords: documentation
Cc: thijs, jessica.mckellar@…, tom@… Branch: branches/basechat-docstrings-2458-2
(diff, github, buildbot, log)
Author: jesstess Launchpad Bug:

Description

They're sparse and missing in some places.

Change History (15)

comment:1 Changed 6 years ago by thijs

  • Cc thijs added
  • Keywords documentation added

comment:2 Changed 5 years ago by jesstess

  • Author set to jesstess
  • Branch set to branches/basechat-docstrings-2458

(In [28803]) Branching to 'basechat-docstrings-2458'

comment:3 Changed 5 years ago by jesstess

  • Owner changed from exarkun to jesstess

comment:4 Changed 5 years ago by jesstess

  • Cc jessica.mckellar@… added
  • Keywords review added
  • Owner jesstess deleted

The branch:

  • Puts docstring quotes on lines by themselves
  • removes debugging print statements
  • remove commented code
  • use standard-compliant spacing between functions and between classes.
  • adds missing epytext markup to all the classes and functions

comment:5 Changed 5 years ago by exarkun

  • Keywords review removed
  • Owner set to jesstess

Thanks!

  1. I think that ContactsList.contacts and ContactsList.onlineContacts probably have IPerson provider values.
  2. A few places give the type of a variable as an interface. This is incomplete; what is probably meant is that the variable must provide the interface, so it should say that. eg, instead of @type client: L{Client<interfaces.IClient>} it should say @type client: L{Client<interfaces.IClient>} provider.
  3. GroupConversation.group is documented a couple times as an implementer, but probably provider is correct.
  4. Likewise for ChatUI..persons, ChatUI.groups, and ChatUI.onlineClients. Instead of are interface, say provide interface.
  5. The docstring for ChatUI.__init__ looks mostly redundant with the information in the docstring for ChatUI. Also, it's documenting parameters that ChatUI.__init__ doesn't even accept.
  6. It looks like ChatUI.getConversation ends with some invalid epytext, @returns The conversation window.. Though, for some reason, this didn't cause the doc builder to go red. Erf. Same issue in getGroupConversation.
  7. I think we mildly prefer @return: over @returns:, too.

comment:6 Changed 5 years ago by jesstess

  • Keywords review added
  • Owner jesstess deleted

Thanks for the review, exarkun. Points 1-7 above should now be addressed in the branch.

comment:7 Changed 4 years ago by exarkun

  • Keywords review removed
  • Owner set to jesstess

Looks good. The news fragment can be a .doc now and the text should probably follow the documented convention, otherwise I have no changes to suggest. The branch does appear to have been created at a point when some conch tests were failing. It'd probably be good to merge forward and run the new branch on buildbot, just to make sure everything's working right. Assuming that goes well, please merge.

comment:8 Changed 4 years ago by binjured

  • Cc tom@… added

Pinging this to see if jesstess intends to make these final minor changes or if it should be unassigned. Thanks for all the work thus far!

comment:9 Changed 4 years ago by <automation>

  • Owner jesstess deleted

comment:10 Changed 3 years ago by jesstess

  • Branch changed from branches/basechat-docstrings-2458 to branches/basechat-docstrings-2458-2

(In [33224]) Branching to 'basechat-docstrings-2458-2'

comment:11 Changed 3 years ago by thijs

Use C{dict} instead of L{dict}.

comment:12 Changed 3 years ago by jesstess

  • Keywords review added

Merged forward and fixed some pydoctor warnings.

Build results here.

comment:13 Changed 3 years ago by exarkun

  • Keywords review removed
  • Owner set to jesstess

Thanks.

  1. I don't like the L{Person<interfaces.IPerson>} convention established in this module. Let's just go with the unsurprising L{IPerson<interfaces.IPerson>}.
  2. ContactsList's class docstring uses param where it should use ivar
  3. Conversation.sendText and GroupConversation.sendText appear to lie about their return types. They each return None but claim to return a Deferred.
  4. GroupConversation.setTopic uses @param: where it means @param topic
  5. in ChatUI.getConversation, Class is documented as an IConversation provider class. A class the instances of which provide an interface is said to implement that interface, so please change this to IConversation implementor.
  6. Likewise for getGroupConversation

Thanks! Please merge when you're happy with the resolution to these points.

comment:14 Changed 3 years ago by jesstess

(In [33259]) Address review comments by exarkun.

refs #2458.

comment:15 Changed 3 years ago by jesstess

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

(In [33260]) Merge basechat-docstrings-2458-2

Author: jesstess
Reviewer: exarkun
Fixes: #2458

Improve the twisted.words.im.basechat API documentation.

Note: See TracTickets for help on using tickets.