Opened 11 years ago
Closed 9 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 )
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 10 years ago by
Description: | modified (diff) |
---|---|
Owner: | set to Itamar Turner-Trauring |
Status: | new → assigned |
Summary: | Client endpoints' connect() should accept a IProtocol provider → Add API to endpoints for connecting a protocol instance |
comment:2 Changed 10 years ago by
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 10 years ago by
I'm not adding a new method, I'm adding a new module-level function, connectProtocol(endpoint, protocol)
.
comment:4 Changed 10 years ago by
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 10 years ago by
Author: | → itamarst |
---|---|
Branch: | → branches/connectprotocol-5270 |
(In [36495]) Branching to 'connectprotocol-5270'
comment:6 Changed 10 years ago by
Keywords: | review added |
---|---|
Owner: | Itamar Turner-Trauring deleted |
Status: | assigned → new |
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 10 years ago by
Keywords: | review removed |
---|---|
Owner: | set to Itamar Turner-Trauring |
Thanks. I guess Twisted programmers everywhere will be singing your praises shortly.
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.- 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. - arghlhtlje
_endpointspy3.py
... Does any new code really belong here? I am thinking about how we have a plan to hopefully recover the history ofendpoints.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 intoendpoints.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? - 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. - I wonder if
connectProtocol
is the best name. It's not too bad, but part of me really wants to insert aWith
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:10 Changed 9 years ago by
Branch: | branches/connectprotocol-5270 → branches/connectprotocol-5270-2 |
---|
(In [37095]) Branching to 'connectprotocol-5270-2'
comment:11 Changed 9 years ago by
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:
- Done.
- Done (
ampclient.py
). - No longer relevant.
- I switched one test to use
MemoryReactor
. The other test seemed like it'd be more complex using that, and convertingfakeendpoints
to meet my needs would be quite a bit more work. - I left the function as
connectProtocol
, since I was unable to come up with a name that was sufficiently better. Suggestions?
comment:12 Changed 9 years ago by
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 indoc/core/howto/endpoints
, and then explain the use of factories after, rather than haveconnectProtocol
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:14 Changed 9 years ago by
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 9 years ago by
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 9 years ago by
Keywords: | review removed |
---|---|
Owner: | set to Itamar Turner-Trauring |
Looks good. Merge away.
comment:17 Changed 9 years ago by
Author: | itamarst → itamarst, tomprince |
---|---|
Branch: | branches/connectprotocol-5270-2 → branches/connectprotocol-5270-3 |
(In [38282]) Branching to connectprotocol-5270-3.
comment:18 Changed 9 years ago by
Resolution: | → fixed |
---|---|
Status: | new → closed |
(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.
Probably better than modifying
connect()
, which wouldn't be backwards compatible, is adding a new API.