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

[multani]

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.

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

[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?

[exarkun]

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

[exarkun]

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

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

[jesstess]

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

[exarkun]

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