Opened 5 years ago

Closed 4 years ago

#5909 enhancement closed fixed (fixed)

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

Reported by: Jean-Paul Calderone Owned by: Jean-Paul Calderone
Priority: normal Milestone:
Component: core Keywords: documentation easy
Cc: Branch: branches/reflect-docs-5909
branch-diff, diff-cov, branch-cov, buildbot
Author: exarkun

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 George Ehrhorn 5 years ago.
patch file for bug #5909

Download all attachments as: .zip

Change History (14)

comment:1 Changed 5 years ago by Itamar Turner-Trauring

You should probably also document curClass argument is private.

Changed 5 years ago by George Ehrhorn

Attachment: patch (bug 5909).txt added

patch file for bug #5909

comment:2 Changed 5 years ago by George Ehrhorn

Resolution: fixed
Status: newclosed

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

comment:3 Changed 5 years ago by George Ehrhorn

Resolution: fixed
Status: closedreopened

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

comment:4 Changed 5 years ago by George Ehrhorn

Keywords: review added

comment:5 Changed 4 years ago by Tom Prince

Keywords: review removed
Owner: set to George Ehrhorn
Status: reopenednew

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 4 years ago by Jean-Paul Calderone

Which patch?

comment:7 Changed 4 years ago by Tom Prince

The attached patch, that was submitted for review.

comment:8 Changed 4 years ago by Jean-Paul Calderone

Author: exarkun
Branch: branches/reflect-docs-5909

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

comment:9 Changed 4 years ago by Jean-Paul Calderone

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

refs #5909

comment:10 Changed 4 years ago by Jean-Paul Calderone

Keywords: review added
Owner: George Ehrhorn 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 4 years ago by Tom Prince

Keywords: review removed
Owner: set to Jean-Paul Calderone

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 4 years ago by Jean-Paul Calderone

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 4 years ago by Jean-Paul Calderone

Resolution: fixed
Status: newclosed

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