Ticket #2458 enhancement closed fixed

Opened 7 years ago

Last modified 2 years ago

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

1

Changed 6 years ago by thijs

  • cc thijs added
  • keywords documentation added

2

Changed 4 years ago by jesstess

  • branch set to branches/basechat-docstrings-2458
  • branch_author set to jesstess

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

3

Changed 4 years ago by jesstess

  • owner changed from exarkun to jesstess

4

Changed 4 years ago by jesstess

  • cc jessica.mckellar@… added
  • keywords documentation, review added; documentation removed
  • 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

5

Changed 4 years ago by exarkun

  • owner set to jesstess
  • keywords documentation added; documentation, review removed

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.

6

Changed 4 years ago by jesstess

  • keywords documentation, review added; documentation removed
  • owner jesstess deleted

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

7

Changed 4 years ago by exarkun

  • owner set to jesstess
  • keywords documentation added; documentation, review removed

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.

8

Changed 3 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!

9

Changed 3 years ago by <automation>

  • owner jesstess deleted

10

Changed 2 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'

11

Changed 2 years ago by thijs

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

12

Changed 2 years ago by jesstess

  • keywords documentation, review added; documentation removed

Merged forward and fixed some pydoctor warnings.

Build results  here.

13

Changed 2 years ago by exarkun

  • owner set to jesstess
  • keywords documentation added; documentation, review removed

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.

14

Changed 2 years ago by jesstess

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

refs #2458.

15

Changed 2 years ago by jesstess

  • status changed from new to closed
  • resolution set to fixed

(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.