Opened 4 years ago
Last modified 3 years ago
See attached patch.
Attaching updated patch.
Okay, I've had a good look at the patch, and I'm satisfied that it does what it says it does. I have two concerns.
One: I want to run this past glyph or exarkun, just to make sure that this code is worth going into Twisted. It looks very similar to reactor.callLater, I feel there should be one obvious way of doing things. Should we deprecate callLater and point to deferLater? Or should we put guidelines in here for where deferLater should be used instead of callLater.
Two: def _callLater(self, *a, **kw): should probably be written: def __callLater(self, delay, func):. The function is private, and is only invoked like this: self._callLater(delay, lambda: d.callback(None)).
Please update the patch with the above modifications, and I'll take responsibility for merging the updated patch if/when I get approval/guidance.
The double underscore in front of __callLater in my comment is a typo. It should be a single underscore.
Branch created: task-deferlater-1875
I've altered the twisted.web2.test.test_http tests to import deferLater, and use that instead. This has managed to reduce the amount of code required, which is highly satisfactory.
More tests for deferLater would be good - we don't currently have a test for if the function raises an error and fires an errback.
I'm confused, why does it do an addCallback for you? That seems unnecessary and redundant. I'd suggest using the function as it was in test_http. Simple, clean, no extraneous gunk.
def deferLater(secs):
d = defer.Deferred() reactor.callLater(secs, d.callback, None) return d
What James said.
Why is
deferLater(n).addCallback(lambda ign: f(x))
preferable to
deferLater(n, f, x)
?
How about a compromise? The function can default to lambda: None, then both of the above will work.
It's preferable for simplicity of API. The deferred is the primary concept here, and it has a perfectly good API already which there's no good reason to hide.
It's not perfectly good. The feature it is missing in this case is invoking a callback without passing any arguments. This is what is addressed by allowing a function and arguments to be passed directly to deferLater.
Same thing is true for all deferred callback functions. This shouldn't be special.
But there's never any possibility of a useful value being passed in here. The fact that the call is being made is the entirety of the information being delivered.
On the mailing list today, I asked about a way to get the result of a task scheduled with callLater. deferLater nearly solves that problem.
As for the API, it would be easy to allow both variants: simply change the callable argument to default to None::
def __call__(self, delay, callable_=None, *args, **kw): d = defer.Deferred() if callable_ is not None: d.addCallback(lambda dummy: callable(*args, **kw)) self._callLater(delay, lambda: d.callback(None)) return d
If there has to be only one way to call it, I think that deferLater(n, f, x) is much better, for the same reasons as exarkun mentioned.
What I am missing is a way to get to the DelayedCall instance returned by callLater. I would simply add a task attribute to the returned Deferred::
... d.task = self._callLater(delay, lambda: d.callback(None)) return d
but I am not sure if this is nice enough.
Can we reach an agreement on this? We can probably have the both interfaces, one basic and another a little more high-level?
ping!
Hmm, I'm thinking about the issue raised by strank now. What if you want to cancel the deferLater's DelayedCall?
Hmm, I think I heard something about adding cancelable Deferreds at some point. Of course, that still doesn't let you reschedule/etc, but that seems okay to me: the more verbose API is still available for such special uses.
If it helps, I'll just state I don't care anymore which of the API alternatives is added, they're really both fine. Just, please don't add both or a hybrid, in some attempt to please me.
(In [22507]) Branching to 'task-deferlater-1875-2'
Okay
Missed a thing...
Missed thing was r22515.
Doh, I messed that up pretty good. :)
Fixed those things.
Please merge.
(In [22548]) Merge task-deferlater-1875-2
Author: mithrandi, jerub, exarkun Reviewer: therve Fixes #1875
Add twisted.internet.task.deferLater, an API for scheduling a future call and getting a Deferred which is called back with its result.