Opened 2 years ago

Closed 21 months ago

#5909 enhancement closed fixed (fixed)

The docstrings for `prefixedMethods` and `accumulateMethods` need some polish

Reported by: exarkun Owned by: exarkun
Priority: normal Milestone:
Component: core Keywords: documentation easy
Cc: Branch: branches/reflect-docs-5909
(diff, github, buildbot, log)
Author: exarkun Launchpad Bug:

Description

A few issues:

  1. prefixedMethods is only documented via a sentence fragment
  2. prefixedMethods has no parameter or (clear) return value documentation
  3. accumulateMethods duplicates its signature in its docstring
  4. accumulateMethods used the deprecated first-person convention
  5. accumulateMethods has no parameter documentation

Attachments (1)

patch (bug 5909).txt (1.1 KB) - added by gehrhorn 2 years ago.
patch file for bug #5909

Download all attachments as: .zip

Change History (14)

comment:1 Changed 2 years ago by itamar

You should probably also document curClass argument is private.

Changed 2 years ago by gehrhorn

patch file for bug #5909

comment:2 Changed 2 years ago by gehrhorn

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

I improved the documentation for prefiedMethods and accumulateMethods and attached a patch.

comment:3 Changed 2 years ago by gehrhorn

  • Resolution fixed deleted
  • Status changed from closed to reopened

I set the status to "fixed", which I don't thing I was supposed to do.

comment:4 Changed 2 years ago by gehrhorn

  • Keywords review added

comment:5 Changed 21 months ago by tom.prince

  • Keywords review removed
  • Owner set to gehrhorn
  • Status changed from reopened to new

This patch deals with points 1, 3 and 4.

The arguments and return value should have explicit documentation (with @param and @return). See the example here. There are also several examples in the file.

Having added explicit argument and return documentation, the description of the behavior could perhaps be simplified (it seems to me that the wording is more complex than necessary in order to describe the arguments inline), but this is optional.

comment:6 Changed 21 months ago by exarkun

Which patch?

comment:7 Changed 21 months ago by tom.prince

The attached patch, that was submitted for review.

comment:8 Changed 21 months ago by exarkun

  • Author set to exarkun
  • Branch set to branches/reflect-docs-5909

(In [36632]) Branching to 'reflect-docs-5909'

comment:9 Changed 21 months ago by exarkun

(In [36633]) Apply patch%20%28bug%205909%29.txt

refs #5909

comment:10 Changed 21 months ago by exarkun

  • Keywords review added
  • Owner gehrhorn deleted

I applied the patch to the linked branch and made some further doc improvements to it. Latest twistedchecker build and latest documentation build.

comment:11 Changed 21 months ago by tom.prince

  • Keywords review removed
  • Owner set to exarkun

Please commit after fixing points 2+3 (and optionally 1).

  1. prefixedMethodNames only refers to a specific class, but addMethodNamesToDict refers to the class and all its base classes, though one is implemented in terms of the other. Also, addMethodNamesToDict looks like an implementation detail (I've filed #6227 to deprecate it).
  2. addMethodNamesToDict doesn't return anything.
  3. accumulateMethodNames almost always wants to be called as
    methodsDict = {}
    reflect.accumulateMethods(object, methodsDict, '<prefix>_')
    # call methods in methodsDict
    
    But it isn't clear from the documentation that that is how it should be called. An example would be helpful here.

comment:12 Changed 21 months ago by exarkun

Thanks. Fixed points 1 and 2 in r36654 and r36655 respectively. I think point 3 is scope creep and I'm inclined not to bother with it for this ticket (these are crappy APIs, anyway, that hardly no one ever cares about or uses).

comment:13 Changed 21 months ago by exarkun

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

(In [36656]) Merge reflect-docs-5909

Author: gehrhorn, exarkun
Reviewer: tom.prince
Fixes: #5909

Bring the API documentation for some twisted.python.reflect functions up
to modern standards.

Note: See TracTickets for help on using tickets.