Opened 16 years ago

Closed 14 years ago

#1875 enhancement closed fixed (fixed)

twisted.internet.task.deferLater()

Reported by: Tristan Seligmann Owned by:
Priority: highest Milestone:
Component: core Keywords:
Cc: jknight, Jean-Paul Calderone, radix, Stephen Thorne, Tristan Seligmann, strank, therve, yaubi Branch: branches/task-deferlater-1875-2
branch-diff, diff-cov, branch-cov, buildbot
Author: exarkun

Description

See attached patch.

Attachments (1)

deferlater.diff (3.2 KB) - added by Tristan Seligmann 16 years ago.

Download all attachments as: .zip

Change History (31)

comment:1 Changed 16 years ago by Tristan Seligmann

Summary: {{{twisted.internet.task.deferLater()}}}twisted.internet.task.deferLater()

comment:2 Changed 16 years ago by Tristan Seligmann

Owner: changed from Jean-Paul Calderone to Stephen Thorne

Attaching updated patch.

Changed 16 years ago by Tristan Seligmann

Attachment: deferlater.diff added

comment:3 Changed 16 years ago by Stephen Thorne

Keywords: review removed
Owner: changed from Stephen Thorne to Tristan Seligmann

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.

comment:4 Changed 16 years ago by Stephen Thorne

The double underscore in front of __callLater in my comment is a typo. It should be a single underscore.

comment:5 Changed 16 years ago by Stephen Thorne

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.

comment:6 Changed 16 years ago by jknight

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

comment:7 Changed 15 years ago by radix

What James said.

comment:8 Changed 15 years ago by Jean-Paul Calderone

Why is

deferLater(n).addCallback(lambda ign: f(x))

preferable to

deferLater(n, f, x)

?

comment:9 Changed 15 years ago by Jean-Paul Calderone

Cc: jknight Jean-Paul Calderone radix Stephen Thorne Tristan Seligmann added

How about a compromise? The function can default to lambda: None, then both of the above will work.

comment:10 Changed 15 years ago by jknight

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.

comment:11 Changed 15 years ago by Jean-Paul Calderone

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.

comment:12 Changed 15 years ago by jknight

Same thing is true for all deferred callback functions. This shouldn't be special.

comment:13 Changed 15 years ago by Jean-Paul Calderone

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.

comment:14 Changed 15 years ago by strank

Cc: strank added

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.

comment:15 Changed 15 years ago by therve

Cc: therve added

Can we reach an agreement on this? We can probably have the both interfaces, one basic and another a little more high-level?

comment:16 Changed 15 years ago by therve

ping!

comment:17 Changed 15 years ago by yaubi

Cc: yaubi added

comment:18 Changed 14 years ago by Tristan Seligmann

Owner: Tristan Seligmann deleted

comment:19 Changed 14 years ago by Jean-Paul Calderone

Hmm, I'm thinking about the issue raised by strank now. What if you want to cancel the deferLater's DelayedCall?

comment:20 Changed 14 years ago by jknight

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.

comment:21 Changed 14 years ago by jknight

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.

comment:22 Changed 14 years ago by Jean-Paul Calderone

author: exarkun
Branch: branches/task-deferlater-1875-2

(In [22507]) Branching to 'task-deferlater-1875-2'

comment:23 Changed 14 years ago by Jean-Paul Calderone

Keywords: review added

Okay

comment:24 Changed 14 years ago by Jean-Paul Calderone

Keywords: review removed

Missed a thing...

comment:25 Changed 14 years ago by Jean-Paul Calderone

Keywords: review added

Missed thing was r22515.

comment:26 Changed 14 years ago by therve

Keywords: review removed
Owner: set to Jean-Paul Calderone
  • deferLater doesn't document its clock argument
  • in writing-standard.xhtml, the example is wrong because it doesn't pass the reactor as argument

comment:27 Changed 14 years ago by Jean-Paul Calderone

Keywords: review added
Owner: Jean-Paul Calderone deleted

Doh, I messed that up pretty good. :)

Fixed those things.

comment:28 Changed 14 years ago by therve

Keywords: review removed
Owner: set to Jean-Paul Calderone

Please merge.

comment:29 Changed 14 years ago by Jean-Paul Calderone

Resolution: fixed
Status: newclosed

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

comment:30 Changed 11 years ago by <automation>

Owner: Jean-Paul Calderone deleted
Note: See TracTickets for help on using tickets.