Adds a callback to Future that is invoked upon completion, whether success or failure.

Adds a callback to Future that is invoked upon completion, whether success or failure.
I'm not totally happy with the method name (in an ideal world, I'd like to call this
method then(), and have onSuccess and onFailure). But this should be backwards-compatible.

Committed: https://code.google.com/p/dart/source/detail?r=8303

[Jennifer Messerly, 2012-06-04 20:11:16]

https://chromiumcodereview.appspot.com/10517006/diff/1/corelib/src/future.dart
File corelib/src/future.dart (right):

https://chromiumcodereview.appspot.com/10517006/diff/1/corelib/src/future.dar...
corelib/src/future.dart:45: * When this future is complete (either with a value
or with an exception),
It'd be good to comment on the order that onComplete, then, and handleException
are called

https://chromiumcodereview.appspot.com/10517006/diff/1/corelib/src/implementa...
File corelib/src/implementation/future_implementation.dart (right):

https://chromiumcodereview.appspot.com/10517006/diff/1/corelib/src/implementa...
corelib/src/implementation/future_implementation.dart:109: for (Function
listener in _completionListeners) {
shouldn't this come after we run _exceptionHandlers and _successListeners, to
match try-catch-finally order?

https://chromiumcodereview.appspot.com/10517006/diff/1/corelib/src/implementa...
corelib/src/implementation/future_implementation.dart:159: this.onComplete((f) {
do you need "this." here?

https://chromiumcodereview.appspot.com/10517006/diff/1/tests/corelib/future_t...
File tests/corelib/future_test.dart (right):

https://chromiumcodereview.appspot.com/10517006/diff/1/tests/corelib/future_t...
tests/corelib/future_test.dart:308:
testExceptionWithCompletionAndSuccessHandlers() {
it'd be good to have a version of "then" and "onComplete" that doesn't throw,
but verifies ordering. (apologies if I missed it)

[Bob Nystrom, 2012-06-04 20:20:39]

Just to let you know, Siggy is on vacation until (I think?) next week.

https://chromiumcodereview.appspot.com/10517006/diff/1/corelib/src/future.dart
File corelib/src/future.dart (right):

https://chromiumcodereview.appspot.com/10517006/diff/1/corelib/src/future.dar...
corelib/src/future.dart:49: void onComplete(void complete(Future<T> future));
How about "always"?

https://chromiumcodereview.appspot.com/10517006/diff/1/corelib/src/implementa...
File corelib/src/implementation/future_implementation.dart (right):

https://chromiumcodereview.appspot.com/10517006/diff/1/corelib/src/implementa...
corelib/src/implementation/future_implementation.dart:109: for (Function
listener in _completionListeners) {
On 2012/06/04 20:11:17, John Messerly wrote:
> shouldn't this come after we run _exceptionHandlers and _successListeners, to
> match try-catch-finally order?

+1

[Jennifer Messerly, 2012-06-04 20:22:27]

Oops, somehow this comment didn't get saved the first time. Resending.

https://chromiumcodereview.appspot.com/10517006/diff/1/corelib/src/implementa...
File corelib/src/implementation/future_implementation.dart (right):

https://chromiumcodereview.appspot.com/10517006/diff/1/corelib/src/implementa...
corelib/src/implementation/future_implementation.dart:192: else
completer.completeException(g.exception);
This is interesting ... it almost suggests we should have a
Completer.completeFuture(Future f) method that does this logic? I wonder if
that'd be useful to anyone else, other than just to implement Future.chain.


style nit: we usually use curlies for if-else pairs.
See http://www.dartlang.org/articles/style-guide/
"DO use curly braces for all flow control structures."

The only exception IIRC is for single line ifs.

[sam.mccall, 2012-06-04 21:35:56]

Thanks! I'll add you guys as reviewers, if it's better to wait for Siggi to come
back that's fine.

I'll make the changes when I'm back at my desk in the morning; but wanted to get
thoughts on the function name.

My perspective is that handleException() is a really awkward mechanism and
then() forces you to use it; so I'm hoping this can be used as a replacement
rather than a complement (at least in my code!).

This may explain my naming concerns & carelessness about ordering ;-)

My secret scheme is that once there's a stable alternate mechanism in place, it
might be less painful to make changes to the then/handleException APIs if people
think it's warranted.

https://chromiumcodereview.appspot.com/10517006/diff/1/corelib/src/future.dart
File corelib/src/future.dart (right):

https://chromiumcodereview.appspot.com/10517006/diff/1/corelib/src/future.dar...
corelib/src/future.dart:45: * When this future is complete (either with a value
or with an exception),
On 2012/06/04 20:11:17, John Messerly wrote:
> It'd be good to comment on the order that onComplete, then, and
handleException
> are called

Agreed, I'll add this to the class doc.

https://chromiumcodereview.appspot.com/10517006/diff/1/corelib/src/future.dar...
corelib/src/future.dart:49: void onComplete(void complete(Future<T> future));
On 2012/06/04 20:20:40, Bob Nystrom wrote:
> How about "always"?

My objection to 'always' is that it mostly makes sense as a contrast with
then/handleException (like try/finally).
Without the contrast, it sounds to me like some sort of repetition or looping
construct, e.g I don't find this obvious:

    file.readText().always((f) {
      print(f.hasValue ? f.value : "Error");
    });

https://chromiumcodereview.appspot.com/10517006/diff/1/corelib/src/implementa...
File corelib/src/implementation/future_implementation.dart (right):

https://chromiumcodereview.appspot.com/10517006/diff/1/corelib/src/implementa...
corelib/src/implementation/future_implementation.dart:109: for (Function
listener in _completionListeners) {
On 2012/06/04 20:20:40, Bob Nystrom wrote:
> On 2012/06/04 20:11:17, John Messerly wrote:
> > shouldn't this come after we run _exceptionHandlers and _successListeners,
to
> > match try-catch-finally order?
> 
> +1

Yeah, that's probably less surprising. When e.g. a success listener throws, the
completion listeners will be interleaved between the success listener and the
corresponding catch block, but that's probably not a big deal.

https://chromiumcodereview.appspot.com/10517006/diff/1/corelib/src/implementa...
corelib/src/implementation/future_implementation.dart:159: this.onComplete((f) {
On 2012/06/04 20:11:17, John Messerly wrote:
> do you need "this." here?

Nope, oops

https://chromiumcodereview.appspot.com/10517006/diff/1/corelib/src/implementa...
corelib/src/implementation/future_implementation.dart:192: else
completer.completeException(g.exception);
On 2012/06/04 20:22:28, John Messerly wrote:
> This is interesting ... it almost suggests we should have a
> Completer.completeFuture(Future f) method that does this logic? I wonder if
> that'd be useful to anyone else, other than just to implement Future.chain.
Yeah, I can't see any obvious other uses, so I'm not inclined to add it now. It
does seem odd though...
(In Java I'd probably overload complete(T) and complete(Future<T>))

> style nit: we usually use curlies for if-else pairs.
> See http://www.dartlang.org/articles/style-guide/
> "DO use curly braces for all flow control structures."
> 
> The only exception IIRC is for single line ifs.
Oops, yes. This is probably better anyway:
future.onComplete((g) => g.hasValue
  ? completer.complete(g.value)
  : completer.completeException(g.exception));

https://chromiumcodereview.appspot.com/10517006/diff/1/tests/corelib/future_t...
File tests/corelib/future_test.dart (right):

https://chromiumcodereview.appspot.com/10517006/diff/1/tests/corelib/future_t...
tests/corelib/future_test.dart:308:
testExceptionWithCompletionAndSuccessHandlers() {
On 2012/06/04 20:11:17, John Messerly wrote:
> it'd be good to have a version of "then" and "onComplete" that doesn't throw,
> but verifies ordering. (apologies if I missed it)

You didn't; will do.

[Jennifer Messerly, 2012-06-04 22:12:49]

https://chromiumcodereview.appspot.com/10517006/diff/1/corelib/src/future.dart
File corelib/src/future.dart (right):

https://chromiumcodereview.appspot.com/10517006/diff/1/corelib/src/future.dar...
corelib/src/future.dart:49: void onComplete(void complete(Future<T> future));
On 2012/06/04 21:35:57, sam.mccall wrote:
> On 2012/06/04 20:20:40, Bob Nystrom wrote:
> > How about "always"?
> 
> My objection to 'always' is that it mostly makes sense as a contrast with
> then/handleException (like try/finally).
> Without the contrast, it sounds to me like some sort of repetition or looping
> construct, e.g I don't find this obvious:
> 
>     file.readText().always((f) {
>       print(f.hasValue ? f.value : "Error");
>     });

yeah. FWIW, I liked the name onComplete

https://chromiumcodereview.appspot.com/10517006/diff/1/corelib/src/implementa...
File corelib/src/implementation/future_implementation.dart (right):

https://chromiumcodereview.appspot.com/10517006/diff/1/corelib/src/implementa...
corelib/src/implementation/future_implementation.dart:192: else
completer.completeException(g.exception);
On 2012/06/04 21:35:57, sam.mccall wrote:
> On 2012/06/04 20:22:28, John Messerly wrote:
> > This is interesting ... it almost suggests we should have a
> > Completer.completeFuture(Future f) method that does this logic? I wonder if
> > that'd be useful to anyone else, other than just to implement Future.chain.
> Yeah, I can't see any obvious other uses, so I'm not inclined to add it now.
It
> does seem odd though...
> (In Java I'd probably overload complete(T) and complete(Future<T>))
> 
> > style nit: we usually use curlies for if-else pairs.
> > See http://www.dartlang.org/articles/style-guide/
> > "DO use curly braces for all flow control structures."
> > 
> > The only exception IIRC is for single line ifs.
> Oops, yes. This is probably better anyway:
> future.onComplete((g) => g.hasValue
>   ? completer.complete(g.value)
>   : completer.completeException(g.exception));

sgtm

[sam.mccall, 2012-06-05 11:59:02]

Made the suggested changes and expanded the class doc. PTAL.

(If this looks okay, it'd be great if someone could commit it for me :-)

[Jennifer Messerly, 2012-06-05 17:53:32]

On 2012/06/05 11:59:02, sam.mccall wrote:
> Made the suggested changes and expanded the class doc. PTAL.
> 
> (If this looks okay, it'd be great if someone could commit it for me :-)

LGTM!

[Siggi Cherem (dart-lang), 2012-06-06 20:42:49]

Thanks Sam for the patch!

LGTM with comments below.

http://codereview.chromium.org/10517006/diff/11001/corelib/src/future.dart
File corelib/src/future.dart (right):

http://codereview.chromium.org/10517006/diff/11001/corelib/src/future.dart#ne...
corelib/src/future.dart:22: * When a future completes:
say in order somewhere? For instance,
=> When a future completes the following actions happen in order:

http://codereview.chromium.org/10517006/diff/11001/corelib/src/implementation...
File corelib/src/implementation/future_implementation.dart (right):

http://codereview.chromium.org/10517006/diff/11001/corelib/src/implementation...
corelib/src/implementation/future_implementation.dart:75: void then(void
onSuccess(T value)) {
onSuccess => onValue (to match what you had in the future.dart, or fix the other
file)

http://codereview.chromium.org/10517006/diff/11001/tests/corelib/future_test....
File tests/corelib/future_test.dart (right):

http://codereview.chromium.org/10517006/diff/11001/tests/corelib/future_test....
tests/corelib/future_test.dart:394: }
could you add a test here registering exception handlers after calling
.transform and .chain (the precondition that was deleted in the comments in
future.dart) ?

[sammccall, 2012-06-07 23:27:22]

This was already committed, a followup patch with the requested changes is at
http://codereview.chromium.org/10544063/