Add API to endpoints for connecting a protocol instance

[Itamar Turner-Trauring]

In many cases, having a factory for client connections is unnecessary boilerplate. The endpoint client API should be extended to accept a protocol instance. This would additionally make learning Twisted easier, and allow deprecation of ClientCreator.

[Itamar Turner-Trauring]

Probably better than modifying connect(), which wouldn't be backwards compatible, is adding a new API.

[Jean-Paul Calderone]

No, adding a new method is just as incompatible. Watch:

>>> from twisted.internet import reactor
>>> from twisted.internet.protocol import Protocol
>>> from twisted.internet.endpoints import clientFromString
>>> clientFromString(reactor, "tcp:localhost:port=1234").connectUsingProtocol(Protocol())
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
AttributeError: 'TCP4ClientEndpoint' object has no attribute 'connectUsingProtocol'
>>> 

[Itamar Turner-Trauring]

I'm not adding a new method, I'm adding a new module-level function, connectProtocol(endpoint, protocol).

[Jean-Paul Calderone]

Thanks for clarifying! FWIW, I think that's the kind of detail that belongs in a ticket description. Specifications are amazingly useful - particularly when they're good specifications - and good tickets often include good specifications.

[itamarst]

(In [36495]) Branching to 'connectprotocol-5270'

[Itamar Turner-Trauring]

Ready for review, I think.

​http://buildbot.twistedmatrix.com/boxes-supported?branch=/branches/connectprotocol-5270

This provides an implementation of connectProtocol, as mentioned above. I updated the client documentation to use the new API instead of requiring a factory, and moved a paragraph about not requiring client factories to the endpoints howto where it seemed more relevant.

[Jean-Paul Calderone]

Thanks. I guess Twisted programmers everywhere will be singing your praises shortly.

1. ClientCreator hasn't been removed yet (not even deprecated), so I disagree with the deletion of the documentation explaining it (I think we've been over this before?). Documentation should last at least as long as functionality. People may not be writing code like this anymore, but that doesn't mean they won't need to know how to read it.
2. Speaking of reading it, how about converting at least one use of ClientCreator in Twisted to this new API, to get the ball rolling? Plus, it's always nice to see that something actually works for some real use case.
3. arghlhtlje _endpointspy3.py... Does any new code really belong here? I am thinking about how we have a plan to hopefully recover the history of endpoints.py, but I don't think it's possible to do that and keep any coherent history for _endpointspy3.py (there will only be one file after we fix the situation, after all). So this inclines me to want all new development to go into endpoints.py. I guess that makes it hard to know whether the new code is Python 3 compatible, and to maintain that compatibility. But... who cares?
4. Someone tried to be nice and started an endpoint testing module, twisted.internet.test.fakeendpoint. They didn't get very far, but perhaps it would be nice to expand that rather than introduce single-use, untested fakes in new tests? Also, I wonder if fake endpoints are even needed in these tests, or if a real endpoint on a fake reactor would work just as well.
5. I wonder if connectProtocol is the best name. It's not too bad, but part of me really wants to insert a With in the middle. I don't think that actually improves the name, but that doesn't prevent me from being somewhat dissatisfied with the current name.

[itamarst]

(In [36497]) Restore ClientCreator example. Refs #5270

[itamarst]

(In [36498]) Switch an example to use connectProtocol. Refs #5270

[itamarst]

(In [37095]) Branching to 'connectprotocol-5270-2'

[Itamar Turner-Trauring]

New build started: ​https://buildbot.twistedmatrix.com/boxes-supported?branch=/branches/connectprotocol-5270-2 - unfortunately looks like some builders are having issues, so this will need to be rerun at some point.

Review points were addressed thusly:

1. Done.
2. Done (ampclient.py).
3. No longer relevant.
4. I switched one test to use MemoryReactor. The other test seemed like it'd be more complex using that, and converting fakeendpoints to meet my needs would be quite a bit more work.
5. I left the function as connectProtocol, since I was unable to come up with a name that was sufficiently better. Suggestions?

[Tom Prince]

One comment on the documention:

• If, as the documentation in doc/core/howto/clients, connectProtocol is the preferred (by which I mean, the way we want to suggest to people that don't know why they might want something else) way to use client endpoints, then it would make sense to explain that first in doc/core/howto/endpoints, and then explain the use of factories after, rather than have connectProtocol be an aside at the end of the section on clients.
• ​twisted checker error

Otherwise, this looks good. Please resubmit after updating the documentation.

[itamarst]

(In [37513]) Address review comments (refs #5270)

[Itamar Turner-Trauring]

The checker error doesn't strike me as particularly useful; this is test code, not real code, so I am don't think it's worth paying attention to. I did add a docstring, but probably now it will want a rtype or something. Also updated howto.

​http://buildbot.twistedmatrix.com/boxes-supported?branch=/branches/connectprotocol-5270-2

[therve]

I'm not sure how I feel about that, instead of working on making OneShotFactory public, like what #5781 aims to. Maybe those are not incompatible though, if connectProtocol uses the new public class.

[Tom Prince]

Looks good. Merge away.

[Tom Prince]

(In [38282]) Branching to connectprotocol-5270-3.

[Tom Prince]

(In [38285]) Merge connectprotocol-5270-3: Add API to endpoints for connecting a protocol instance.

Author: itamar Reviewers: exarkun, tom.prince Fixes: #5270

In many cases, having a factory for client connections is unnecessary boilerplate. The endpoint client API should be extended to accept a protocol instance. This would additionally make learning Twisted easier, and allow deprecation of ClientCreator.