Opened 17 months ago

Closed 17 months ago

Last modified 17 months ago

#6459 enhancement closed fixed (fixed)

Remove @see docstrings from PosixReactorBase so that full API documentation can be inherited from the appropriate interface

Reported by: rwall Owned by: arbiter
Priority: normal Milestone:
Component: core Keywords: documentation easy
Cc: Branch: branches/posixreactorbase-apidocs-6459
(diff, github, buildbot, log)
Author: rwall Launchpad Bug:

Description

Various methods in PosixReactorBase (source:trunk/twisted/internet/posixbase.py#L267) contain only an @see reference. eg

493	        """@see: twisted.internet.interfaces.IReactorTCP.connectTCP
494	        """

This leads to unhelpful API documentation:

It would be better to completely remove the docstring in such cases, which will cause pydoctor to insert the full documentation from the appropriate interface class.

See ticket:4685#comment:16 for another example and explanation of a similar change to the twisted.names API documentation.

Attachments (2)

ticket#6459.patch (2.3 KB) - added by arbiter 17 months ago.
a possible patch
posixreactorbase-apidocs-6459.patch (2.3 KB) - added by arbiter 17 months ago.
changed the file name to more URL friendly

Download all attachments as: .zip

Change History (13)

comment:1 Changed 17 months ago by arbiter

  • Owner set to arbiter
  • Status changed from new to assigned

Changed 17 months ago by arbiter

a possible patch

comment:2 follow-up: Changed 17 months ago by arbiter

  • Keywords review added
  • Owner arbiter deleted
  • Status changed from assigned to new

I have removed the docstring in such cases under the class PosixReactorBase.

comment:3 in reply to: ↑ 2 Changed 17 months ago by rwall

Replying to arbiter:

I have removed the docstring in such cases under the class PosixReactorBase.

Thanks arbiter,

The # in your patch filename seems to cause problems for trac and prevents me from downloading it.

Can you attach it again with a url friendly filename eg posixreactorbase-apidocs-6459.patch

This doc explains the branch naming policy and I think it's useful if patches follow the same naming pattern. https://twistedmatrix.com/trac/wiki/TwistedWithCombinator#Makinganewbranch:

-RichardW.

Changed 17 months ago by arbiter

changed the file name to more URL friendly

comment:4 Changed 17 months ago by rwall

  • Author set to rwall
  • Branch set to branches/posixreactorbase-apidocs-6459

(In [38212]) Branching to 'posixreactorbase-apidocs-6459'

comment:5 Changed 17 months ago by rwall

(In [38213]) Apply posixreactorbase-apidocs-6459.patch from arbiter. refs #6459

comment:6 follow-up: Changed 17 months ago by rwall

  • Keywords review removed
  • Owner set to arbiter

Code Review

Thanks arbiter.

I downloaded your patch, applied it to a branch.

Your patch didn't contain a News file so I added one for you. Read
about News files here and remember to include one in your next patch:

I ran the Twisted documentation builder on that branch:

... and the resulting API docs look much better.

Looks like you removed all the offending docstrings. Great.

So I'll merge this to trunk.

If you want to help further improve the documentation, you could look
for any other examples of this problem in the API docs and raise some
further tickets.

You might find them by inspection, or perhaps by greping the source
code for docstrings containing only @see references. Then check that
the value is actually pointing to a corresponding superclass or
interface method.

exarkun has noted (ticket:6012#comment:8) that sometimes the @see
should be left in place eg if the docstring contains some useful
content *and* an @see reference. In this case it's important that the
@see value is contained in L{} markup so that pydoctor can create a
suitable link.

So search for cases where the @see value is not contained in L{} and
raise tickets for those too.

If you find lots of these problems try and group them into managable
"ticket" sized chunks (perhaps by module) so that the patches are
small enough to review.

Add a link / reference to this ticket in each new related ticket

And if you're feeling really adventurous you might glance at the
twistedchecker or pydoctor source code and think about how they could
check for these problems automatically.

See also:

-RichardW.

comment:7 Changed 17 months ago by rwall

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

(In [38218]) Merge posixreactorbase-apidocs-6459

Author: arbiter
Reviewer: rwall
Fixes: #6459

Remove solitary @see docstrings from PosixReactorBase so that pydoctor
inserts the complete interface documentation into the API docs.

comment:8 in reply to: ↑ 6 Changed 17 months ago by arbiter

Replying to rwall:

Code Review

Thanks arbiter.

I downloaded your patch, applied it to a branch.

Your patch didn't contain a News file so I added one for you. Read
about News files here and remember to include one in your next patch:

I ran the Twisted documentation builder on that branch:

... and the resulting API docs look much better.

Looks like you removed all the offending docstrings. Great.

So I'll merge this to trunk.

If you want to help further improve the documentation, you could look
for any other examples of this problem in the API docs and raise some
further tickets.

You might find them by inspection, or perhaps by greping the source
code for docstrings containing only @see references. Then check that
the value is actually pointing to a corresponding superclass or
interface method.

exarkun has noted (ticket:6012#comment:8) that sometimes the @see
should be left in place eg if the docstring contains some useful
content *and* an @see reference. In this case it's important that the
@see value is contained in L{} markup so that pydoctor can create a
suitable link.

So search for cases where the @see value is not contained in L{} and
raise tickets for those too.

If you find lots of these problems try and group them into managable
"ticket" sized chunks (perhaps by module) so that the patches are
small enough to review.

Add a link / reference to this ticket in each new related ticket

And if you're feeling really adventurous you might glance at the
twistedchecker or pydoctor source code and think about how they could
check for these problems automatically.

See also:

-RichardW.

Thank you very much for your suggestions. I would be very glad to keep on working on Twisted.

comment:9 follow-up: Changed 17 months ago by exarkun

twisted/internet/iocpreactor/reactor.py follows the same pattern as posixbase.py previously did. If changing one makes sense, changing both probably makes sense.

comment:10 in reply to: ↑ 9 Changed 17 months ago by arbiter

Replying to exarkun:

twisted/internet/iocpreactor/reactor.py follows the same pattern as posixbase.py previously did. If changing one makes sense, changing both probably makes sense.

I have raised a ticket and submitted a patch here about this problem http://twistedmatrix.com/trac/ticket/6483.

comment:11 Changed 17 months ago by glyph

(In [38382]) Taking my lead from #6459, don't document things that already have documentation.

This is an interesting (and good) feature of pydoctor, but it's a little bit
unfortunate while reading the code itself. I wonder if maybe in the future we
ought to implement (or suggest to the Zope Interface maintainers that they
implement) an @implementation decorator that specifically calls out a method
as an implementation of an interface method that should not be documented on its
own.

Refs #6459

Refs #986

Note: See TracTickets for help on using tickets.