Support transforming the exception in Future.transform().

Support transforming the exception in Future.transform().

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

[Bob Nystrom, 2012-08-31 17:39:26]

With normal exceptions, it's common to catch a lower-level exception and rethrow
something higher-level or with a better error message.

Stuff like Future.transform() doesn't currently give you an easy way to do that.
This patch is a stab at addressing that. I want something like this for pub.

The code that is (will be) using it looks like:

    return httpGetString(fullUrl).transform((body) {
      var doc = JSON.parse(body);
      return doc['versions'].map((version) => new Version.parse(version));
    }, (ex) {
      if (ex is HttpException && ex.statusCode == 404) {
        throw 'Could not find package "${parsed.first}" on ${parsed.last}.';
      }
    });

WDYT?

[Siggi Cherem (dart-lang), 2012-09-04 16:56:32]

Looks nice! Nice tests :)

I haven't thought too much about it, but an alternative could be to add a
transformException method that acts on its own. On your example:

 return httpGetString(fullUrl)
    .transform((body) {
      var doc = JSON.parse(body);
      return doc['versions'].map((version) => new Version.parse(version));
    })
    .transformException((ex) {
      if (ex is HttpException && ex.statusCode == 404) {
        throw 'Could not find package "${parsed.first}" on ${parsed.last}.';
      }
    });

This way, you can use the same method for both [transform] and [chain]

Regardless, LGTM with your approach (adding support for chain)

https://chromiumcodereview.appspot.com/10908040/diff/1/lib/core/future.dart
File lib/core/future.dart (right):

https://chromiumcodereview.appspot.com/10908040/diff/1/lib/core/future.dart#n...
lib/core/future.dart:116: Future transform(transformation(T value),
[onException(Object exception)]);
maybe use [bool onException(Object exception)]?
(add bool type?)

https://chromiumcodereview.appspot.com/10908040/diff/1/lib/core/future.dart#n...
lib/core/future.dart:133: Future chain(Future transformation(T value));
+ onException :)

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

https://chromiumcodereview.appspot.com/10908040/diff/1/tests/corelib/future_t...
tests/corelib/future_test.dart:457: completer.complete("original error");
original error => normal value?

[Bob Nystrom, 2012-09-04 18:00:48]

Took your suggestion and made transformException() a separate operation. PTAL
and let me know what you think.

[Siggi Cherem (dart-lang), 2012-09-04 20:17:52]

lgtm

https://chromiumcodereview.appspot.com/10908040/diff/6001/lib/core/future.dart
File lib/core/future.dart (right):

https://chromiumcodereview.appspot.com/10908040/diff/6001/lib/core/future.dar...
lib/core/future.dart:140: * returns anything else, the original exception will
be propagated.
thinking about the control flow of this and how similar it is to try-catch -
maybe the return value should be propagated as the result of the future?

[nweiz, 2012-09-04 20:26:14]

This change seems like a positive incremental movement on the current API, but
I'm worried that it doesn't address the underlying issues that cause problems
like this. It makes one use case of Futures nicer, but it doesn't address many
other similar cases, such as a parallel to `chain` for exceptions, or a way to
handle exceptions and return to the success path.

I've written up an overview of a more radical possible revision for the Future
API that (I hope) gets at the underlying problems:
https://gist.github.com/16a7367636d577efb4dc

https://chromiumcodereview.appspot.com/10908040/diff/6001/lib/core/future.dart
File lib/core/future.dart (right):

https://chromiumcodereview.appspot.com/10908040/diff/6001/lib/core/future.dar...
lib/core/future.dart:142: Future transformException(bool transformation(Object
exception));
It seems like this should just be the default behavior of handleException.

https://chromiumcodereview.appspot.com/10908040/diff/6001/lib/coreimpl/future...
File lib/coreimpl/future_implementation.dart (right):

https://chromiumcodereview.appspot.com/10908040/diff/6001/lib/coreimpl/future...
lib/coreimpl/future_implementation.dart:175: handleException((ex) {
Why are you renaming this to "ex"? "catch (e)" is much more widely used in Dart
code.

[Bob Nystrom, 2012-09-04 21:14:43]

Thanks!

https://chromiumcodereview.appspot.com/10908040/diff/6001/lib/core/future.dart
File lib/core/future.dart (right):

https://chromiumcodereview.appspot.com/10908040/diff/6001/lib/core/future.dar...
lib/core/future.dart:140: * returns anything else, the original exception will
be propagated.
On 2012/09/04 20:17:52, sigmund wrote:
> thinking about the control flow of this and how similar it is to try-catch -
> maybe the return value should be propagated as the result of the future?

Works for me. Done.

https://chromiumcodereview.appspot.com/10908040/diff/6001/lib/core/future.dar...
lib/core/future.dart:142: Future transformException(bool transformation(Object
exception));
On 2012/09/04 20:26:14, nweiz wrote:
> It seems like this should just be the default behavior of handleException.

I'm trying to keep this change as minimally invasive as possible, so I don't
want to change the behavior of existing methods. How about I ignore this for now
and after M1 we look at a revamp?

https://chromiumcodereview.appspot.com/10908040/diff/6001/lib/coreimpl/future...
File lib/coreimpl/future_implementation.dart (right):

https://chromiumcodereview.appspot.com/10908040/diff/6001/lib/coreimpl/future...
lib/coreimpl/future_implementation.dart:175: handleException((ex) {
On 2012/09/04 20:26:14, nweiz wrote:
> Why are you renaming this to "ex"? "catch (e)" is much more widely used in
Dart
> code.

Stale change. Removed.

[nweiz, 2012-09-04 21:35:34]

lgtm

https://chromiumcodereview.appspot.com/10908040/diff/6001/lib/core/future.dart
File lib/core/future.dart (right):

https://chromiumcodereview.appspot.com/10908040/diff/6001/lib/core/future.dar...
lib/core/future.dart:142: Future transformException(bool transformation(Object
exception));
On 2012/09/04 21:14:43, Bob Nystrom wrote:
> On 2012/09/04 20:26:14, nweiz wrote:
> > It seems like this should just be the default behavior of handleException.
> 
> I'm trying to keep this change as minimally invasive as possible, so I don't
> want to change the behavior of existing methods. How about I ignore this for
now
> and after M1 we look at a revamp?

Sure, seems fine.