Opened 6 years ago

Closed 4 years ago

#5270 enhancement closed fixed (fixed)

Add API to endpoints for connecting a protocol instance

Reported by: Itamar Turner-Trauring Owned by: Itamar Turner-Trauring
Priority: low Milestone:
Component: core Keywords:
Cc: Branch: branches/connectprotocol-5270-3
branch-diff, diff-cov, branch-cov, buildbot
Author: itamarst, tomprince

Description (last modified by 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.

Change History (18)

comment:1 Changed 5 years ago by Itamar Turner-Trauring

Description: modified (diff)
Owner: set to Itamar Turner-Trauring
Status: newassigned
Summary: Client endpoints' connect() should accept a IProtocol providerAdd API to endpoints for connecting a protocol instance

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

comment:2 Changed 5 years ago by 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'
>>> 

comment:3 Changed 5 years ago by Itamar Turner-Trauring

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

comment:4 Changed 5 years ago by 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.

comment:5 Changed 5 years ago by itamarst

Author: itamarst
Branch: branches/connectprotocol-5270

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

comment:6 Changed 5 years ago by Itamar Turner-Trauring

Keywords: review added
Owner: Itamar Turner-Trauring deleted
Status: assignednew

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.

comment:7 Changed 5 years ago by Jean-Paul Calderone

Keywords: review removed
Owner: set to Itamar Turner-Trauring

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.

comment:8 Changed 5 years ago by itamarst

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

comment:9 Changed 5 years ago by itamarst

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

comment:10 Changed 5 years ago by itamarst

Branch: branches/connectprotocol-5270branches/connectprotocol-5270-2

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

comment:11 Changed 5 years ago by Itamar Turner-Trauring

Keywords: review added
Owner: Itamar Turner-Trauring deleted

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?

comment:12 Changed 5 years ago by Tom Prince

Keywords: review removed
Owner: set to Itamar Turner-Trauring

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.

comment:13 Changed 4 years ago by itamarst

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

comment:14 Changed 4 years ago by Itamar Turner-Trauring

Keywords: review added
Owner: Itamar Turner-Trauring deleted

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

comment:15 Changed 4 years ago by 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.

comment:16 Changed 4 years ago by Tom Prince

Keywords: review removed
Owner: set to Itamar Turner-Trauring

Looks good. Merge away.

comment:17 Changed 4 years ago by Tom Prince

Author: itamarstitamarst, tomprince
Branch: branches/connectprotocol-5270-2branches/connectprotocol-5270-3

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

comment:18 Changed 4 years ago by Tom Prince

Resolution: fixed
Status: newclosed

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

Note: See TracTickets for help on using tickets.