Ticket #5131 defect closed fixed

Opened 3 years ago

Last modified 3 years ago

t.i.p.BaseProtocol mentions unknow 'dataReceived'

Reported by: multani Owned by: exarkun
Priority: normal Milestone:
Component: core Keywords: documentation
Cc: jesstess Branch: branches/baseprotocol-docstring-5131
(diff, github, buildbot, log)
Author: exarkun Launchpad Bug:

Description

Reading the API documentation of t.i.p.BaseProtocol, it says that:

... My API is quite simple. Implement dataReceived(data) ...

But dataReceived() is actually implemented by the subclass in t.i.p.Protocol.

If you read the documentation of BaseProtocol only, there's no way (AFAIK) that dataReceived() implemented by your subclass is going to be called. So I think andthe docstring of BaseProtocol should be updated not to mention dataReceived(). Otherwise, it just confuses reader.

Change History

1

follow-up: ↓ 2   Changed 3 years ago by exarkun

Well... It's true that BaseProtocol is shared between Protocol, ProcessProtocol, and DatagramProtocol. As such, the mention of the dataReceived method is somewhat out of place.

I'm not sure If you read the documentation of BaseProtocol only, there's no way (AFAIK) that dataReceived() implemented by your subclass is going to be called. makes sense, though. dataReceived is called on Protocol instances that receive data.

2

in reply to: ↑ 1   Changed 3 years ago by multani

Replying to exarkun:

Well... It's true that BaseProtocol is shared between Protocol, ProcessProtocol, and DatagramProtocol. As such, the mention of the dataReceived method is somewhat out of place.

Can't this be just moved to the t.i.p.Protocol docstring instead, and update BaseProtocol docstring to say to refer to subclass for the way of reading data out of the transport?

I'm not sure If you read the documentation of BaseProtocol only, there's no way (AFAIK) that dataReceived() implemented by your subclass is going to be called. makes sense, though. dataReceived is called on Protocol instances that receive data.

Actually, I was thinking that if you passed a BaseProtocol instance, dataReceived() would not be called at all, but I was wrong (this is the transport which calls this). That's also mean that if you implement this method, there are great chances that you are going to use a transport which expect a IProtocol, which should also have connectionLost(), right?

3

  Changed 3 years ago by exarkun

  • branch set to branches/baseprotocol-docstring-5131
  • branch_author set to exarkun

(In [32262]) Branching to 'baseprotocol-docstring-5131'

4

follow-up: ↓ 5   Changed 3 years ago by exarkun

  • keywords review added

Thanks for the additional feedback multani. I've checked some doc changes into the branch.

5

in reply to: ↑ 4   Changed 3 years ago by multani

I think this is much better than before: it removes the confusion from BaseProtocol and add more documentation to the (more directly useful) Protocol class.

I consider the original report to be fixed with those changes.

6

  Changed 3 years ago by jesstess

  • keywords review removed
  • cc jesstess added
  • owner set to exarkun

Thanks for working on this, exarkun. Looks great, please merge.

7

  Changed 3 years ago by exarkun

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

(In [32336]) Merge baseprotocol-docstring-5131

Author: exarkun Reviewer: multani, jesstess Fixes: #5131

Give Protocol a class docstring, mostly taken from BaseProtocol's docstring, which was really describing a lot of things particular to Protocol. Also give BaseProtocol a docstring more appropriate to it.

Note: See TracTickets for help on using tickets.