Opened 8 years ago

Closed 6 years ago

#2375 defect closed fixed (fixed)

these objects' docstrings are not proper epytext:

Reported by: radix Owned by:
Priority: normal Milestone:
Component: core Keywords: documentation
Cc: mwh, thijs, therve, exarkun Branch: branches/proper-epytext-2375
(diff, github, buildbot, log)
Author: thijs Launchpad Bug:

Description

these objects' docstrings are not proper epytext:

twisted.python.modules
twisted.python.modules._ModuleIteratorHelper.getitem
twisted.python.modules._ModuleIteratorHelper.getitem
twisted.python.zshcomp
twisted.vfs.backends.adhoc.AdhocDirectory.child
twisted.vfs.backends.adhoc.AdhocDirectory.child
twisted.vfs.backends.inmem.FakeDirectory.child
twisted.vfs.backends.inmem.FakeDirectory.child
twisted.vfs.ivfs.IFileSystemContainer.child
twisted.vfs.ivfs.IFileSystemContainer.child
twisted.words.protocols.jabber.client.XMPPClientFactory
twisted.words.protocols.jabber.client.XMPPClientFactory
twisted.words.xish.domish.Element
twisted.web2.dav.xattrprops.xattrPropertyStore
twisted.web2.dav.resource.DAVPropertyMixIn
twisted.web2.client.http.EmptyHTTPClientManager.clientPipelining
twisted.web2.client.http.EmptyHTTPClientManager.clientPipelining
twisted.web2.client.http.EmptyHTTPClientManager.clientPipelining
twisted.web2.client.interfaces.IHTTPClientManager.clientPipelining
twisted.web2.client.interfaces.IHTTPClientManager.clientPipelining
twisted.web2.client.interfaces.IHTTPClientManager.clientPipelining
twisted.mail.maildir.StringListMailbox.listMessages
twisted.mail.maildir.StringListMailbox.getMessage
twisted.mail.maildir.StringListMailbox.listMessages
twisted.mail.maildir.StringListMailbox.getMessage
twisted.mail.pop3.IMailbox.listMessages
twisted.mail.pop3.IMailbox.getMessage
twisted.mail.pop3.IMailbox.listMessages
twisted.mail.pop3.IMailbox.getMessage
twisted.mail.pop3.Mailbox.listMessages
twisted.mail.pop3.Mailbox.getMessage
twisted.mail.pop3.Mailbox.listMessages
twisted.mail.pop3.Mailbox.getMessage
twisted.internet.defer.inlineCallbacks
twisted.internet.defer.inlineCallbacks
twisted.internet._dumbwin32proc._findShebang
twisted.internet._dumbwin32proc._findShebang
twisted.spread.jelly.Jellyable.jellyFor
twisted.spread.flavors.RemoteCopy.unjellyFor
twisted.spread.flavors.RemoteCopy.unjellyFor
twisted.spread.flavors.RemoteCache.unjellyFor
twisted.spread.flavors.RemoteCache.unjellyFor
twisted.spread.jelly.Jellyable.jellyFor
twisted.spread.jelly.Jellyable.jellyFor
twisted.spread.jelly.Jellyable.jellyFor
twisted.spread.jelly.Unjellyable.unjellyFor
twisted.spread.jelly.Unjellyable.unjellyFor
twisted.spread.banana.Banana.setPrefixLimit
twisted.spread.banana.Banana.setPrefixLimit

Attachments (2)

proper-epytext-2375.patch (39.6 KB) - added by thijs 6 years ago.
patch against r24262
epy-fix.py (953 bytes) - added by thijs 6 years ago.

Download all attachments as: .zip

Change History (22)

comment:1 Changed 8 years ago by jml

  • Owner changed from glyph to radix

It would be helpful if you could show how you generated this list, and how one might verify that a given object has a valid epytext docstring. That way, I can fix this bug :)

For bonus points, make pyflakes report invalid epytext.

comment:2 Changed 8 years ago by mwh

You get this list when you run PyDoctor from SVN head against Twisted SVN HEAD.

I've been talking (#2272) about setting up a pydoctor buildbot to catch this sort of thing closer to source, but been far too busy to do it. Integrating it into pyflakes shouldn't be very hard and would be almost as good.

comment:3 Changed 8 years ago by mwh

  • Cc mwh added

comment:4 Changed 6 years ago by thijs

  • Cc thijs added
  • Owner changed from radix to thijs
  • Status changed from new to assigned

comment:5 Changed 6 years ago by thijs

List for current trunk (r24264) where 28 objects' docstrings are not proper epytext:

    twisted.mail.test.test_pop3
    twisted.python.test.test_util.PasswordTestingProcessProtocol
    twisted.conch.test.test_conch.ConchTestForwardingProcess.__init__
    twisted.conch.test.test_helper.BufferTestCase.test_linefeed
    twisted.conch.test.test_helper.BufferTestCase.test_newline
    twisted.conch.test.test_insults.ServerProtocolOutputTests.test_nextLine
    twisted.flow.test.test_flow.slowlist
    twisted.flow.test.test_flow.producer
    twisted.flow.test.test_flow.consumer
    twisted.flow.test.test_flow.badgen
    twisted.flow.test.test_flow.buildlist
    twisted.flow.test.test_flow.testconcur
    twisted.flow.test.test_flow.echoServer
    twisted.flow.test.test_flow.echoClient
    twisted.internet.test.test_iocp.IOCPReactorTestCase.test_stopStartReading
    twisted.python.test.test_release.ProjectTest.makeProject
    twisted.python.test.test_release.DistributionBuilderTests.test_subProjectLayout
    twisted.python.test.test_release.DistributionBuilderTests.test_coreProjectLayout
    twisted.test.test_amp.BinaryProtocolTests.test_receiveBoxStateMachine
    twisted.test.test_ftp.FTPClientTestCase.test_lostRETR
    twisted.test.test_internet.TimeTestCase.test_seconds
    twisted.test.test_process.MockProcessTestCase.test_mockForkInParentGarbageCollectorEnabled
    twisted.test.test_process.MockProcessTestCase.test_mockForkInParentGarbageCollectorDisabled
    twisted.test.test_reflect.LookupsTestCase.test_invalidNames
    twisted.trial.test.test_reporter.PyunitTestNames.test_minimalReporter
    twisted.web.test.test_xml.TestBrokenHTML
    twisted.web2.test.test_vhost.TestVhost.testNameVirtualHost
    twisted.web2.test.test_wsgi.TestContainer.flushErrors

Changed 6 years ago by thijs

patch against r24262

comment:6 Changed 6 years ago by thijs

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

The attached patch fixes all epydoc issues found by pydoctor.

comment:7 Changed 6 years ago by therve

  • Cc therve added
  • Owner set to exarkun

I don't understand: exarkun, these errors should be reporter by the documentation buildbot slave, no?

comment:8 follow-up: Changed 6 years ago by exarkun

They seem all to be in tests; I think pydoctor ignores the docstrings of tests.

comment:9 follow-up: Changed 6 years ago by exarkun

  • Keywords review removed
  • Owner changed from exarkun to thijs

So, maybe we want to fix these, but we should get automated coverage so that they remain correct. This means changing the pydoctor model, I think, or extending the buildbot API generation step to check them in some other way.

On the other hand, if we don't actually include test docstrings in the generated API docs, then it doesn't really matter if the markup is correct or not. Perhaps instead we should stop using epytext in test docstrings?

Actually I don't like that idea. I think we should generate API docs for the tests - perhaps separately from the rest of the docs (but perhaps not)? These could be useful for people (for example, people trying to write their own tests for Twisted-using code).

comment:10 in reply to: ↑ 8 Changed 6 years ago by thijs

  • Status changed from new to assigned

Replying to exarkun:

They seem all to be in tests; I think pydoctor ignores the docstrings of tests.

Most of them are except for:

  • twisted/protocols/ftp.py
  • twisted/trial/reporter.py

comment:11 in reply to: ↑ 9 Changed 6 years ago by thijs

  • Keywords review added
  • Owner changed from thijs to exarkun
  • Status changed from assigned to new

Replying to exarkun:

So, maybe we want to fix these, but we should get automated coverage so that they remain correct. This means changing the pydoctor model, I think, or extending the buildbot API generation step to check them in some other way.

Yes, this probably means extending pydoctor and/or the buildbot API generation step. I attached a crappy script that reads a file containing the list of objects reported as broken by pydoctor, and then runs epydoc with warnings enabled on every module, displaying the errors. The script is absolutely not a fix or solution for pydoctor but it worked for me.

On the other hand, if we don't actually include test docstrings in the generated API docs, then it doesn't really matter if the markup is correct or not. Perhaps instead we should stop using epytext in test docstrings? Actually I don't like that idea.

I don't like that idea either.

I think we should generate API docs for the tests - perhaps separately from the rest of the docs (but perhaps not)?

I think it's good to use epytext in the tests, so people can generate pretty docs for them if they need docs for the tests, but I don't see why the majority of the users would need API docs for the tests. So generating API docs for the tests for users isn't that important imo, as long as they can do it themselves. Sacrificing a buildbot and cycles for testing the epydoc strings in the tests seems like overkill.

These could be useful for people (for example, people trying to write their own tests for Twisted-using code).

Assigning back to exarkun for review, not sure what the current status of this ticket is.

Changed 6 years ago by thijs

comment:12 Changed 6 years ago by exarkun

  • Cc exarkun added
  • Keywords review removed
  • Owner changed from exarkun to thijs

Sacrificing a buildbot and cycles for testing the epydoc strings in the tests seems like overkill.

This is basically the core of it for me. If it's not worth making sure it stays right, then I don't feel like it's worth fixing.

So I think we should close this as fixed/wontfix or turn the epy-fix.py into something that can run on buildbot in the documentation builder.

comment:13 follow-up: Changed 6 years ago by thijs

  • Status changed from new to assigned

There are one or two files that don't belong to the tests but contain incorrect epytext, I suggest we fix those and ignore the rest. (that way at least some of all the work I spent to correct it will be merged :)

comment:14 Changed 6 years ago by thijs

I'll double-check if my previous comment is still valid and then fix it, otherwise close the ticket.

comment:15 Changed 6 years ago by thijs

  • author set to thijs
  • Branch set to branches/proper-epytext-2375

(In [25031]) Branching to 'proper-epytext-2375'

comment:16 in reply to: ↑ 13 Changed 6 years ago by thijs

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

Replying to thijs:

There are one or two files that don't belong to the tests but contain incorrect epytext, I suggest we fix those and ignore the rest. (that way at least some of all the work I spent to correct it will be merged :)

Fixed the errors in those 2 files in r25032, putting it up for review.

comment:17 Changed 6 years ago by exarkun

  • Keywords review removed
  • Owner set to thijs

The twisted/protocols/ftp.py changes look good, and I filed a ticket upstream with pydoctor so that the kind of error represented by @param commands will be detected and reported.

Some of the changes to twisted/trial/reporter.py look good. I'm not sure about the changes from *args to args though. We're probably abusing epytext here, but we do this in a lot of places in Twisted, not just reporter.py. Changing just these two instances doesn't fix the overall problem. We need to figure out if *args will actually do something useful, then we can either keep using it or stop using it everywhere. (for reference, grep -E '@param \*' twisted/ -r --include '*.py')

I'd suggest dropping the *args/args change and merging the rest.

comment:18 Changed 6 years ago by thijs

  • Status changed from new to assigned

When I run epydoc in warning mode on reporter.py it throws the following error:

+--------------------------------------------------------------------------------------------------------------------------------------------
| File /trunk/twisted/trial/reporter.py, line 399, in twisted.trial.reporter.Reporter._writeln
|   Warning: @param for unknown parameter "*args"

When I change it back to '@param args' the error goes away and the docs are properly generated with epydoc. pydoctor might already understand this but I think it's a good idea to make it work for both epydoc and pydoctor.

I've changed the 'args' param to '*args' in r25096 and opened a ticket (#3501) that proposes to fix all of these occurrences in the codebase.

comment:19 Changed 6 years ago by thijs

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

(In [25097]) Merge proper-epytext-2375: Fix some incorrect epytext markup in the docstrings.

Author: thijs
Reviewer: exarkun
Fixes: #2375

comment:20 Changed 4 years ago by <automation>

  • Owner thijs deleted
Note: See TracTickets for help on using tickets.