Add typed wrapper functions to delegate classes.

Add typed wrapper functions to delegate classes.

These mirror the methods in the the collection package, and serve a
similar purpose of safely casting generic objects when the user is
confident that the actual object's values are more specific than the
static type.

R=floitsch@google.com, lrn@google.com

Committed: https://github.com/dart-lang/async/commit/641e1954b4c915611e5ea9d9ab871748443253ff

[Lasse Reichstein Nielsen, 2016-04-08 09:20:29]

lgtm

https://codereview.chromium.org/1870543004/diff/1/lib/src/delegate/future.dart
File lib/src/delegate/future.dart (right):

https://codereview.chromium.org/1870543004/diff/1/lib/src/delegate/future.dar...
lib/src/delegate/future.dart:16: /// Creates a wrapper that asserts the types of
the value emitted by [future].
"asserts" isn't clear.
Maybe: 
  Creates a `Future` wrapper which throws if a value result isn't an instance of
`T`.
(or similar and shorter, so it fits in a line).

https://codereview.chromium.org/1870543004/diff/1/lib/src/delegate/future.dar...
lib/src/delegate/future.dart:18: /// This soundly converts a [Future] without a
generic type to a `Future<T>`
You can't have a Future without a generic type - the Future type is generic, so
all Future types have a type parameter, possibly "dynamic".

The function works for any Future<Type>, not just those that are dynamic. It can
be used to *cast* any Future type to another Future type. (We should have a cast
operator on Future doing this instead:
  Future<S> cast<S>();
and probably have that on all generic collections and boxings).

https://codereview.chromium.org/1870543004/diff/1/lib/src/delegate/future.dar...
lib/src/delegate/future.dart:19: /// by asserting that its value is an instance
of `T` whenever it's accessed.
You don't "access" a future value, it's push, not pull.
Maybe use "whenever it's provided"?

https://codereview.chromium.org/1870543004/diff/1/lib/src/delegate/future.dar...
lib/src/delegate/future.dart:20: /// If it's not, it throws a [CastError].
Two "it"s referring to different things in the same sentence, the last one needs
to be written out.

https://codereview.chromium.org/1870543004/diff/1/lib/src/delegate/stream.dart
File lib/src/delegate/stream.dart (right):

https://codereview.chromium.org/1870543004/diff/1/lib/src/delegate/stream.dar...
lib/src/delegate/stream.dart:24: /// accessed. If they're not, it throws a
[CastError].
Same comments as for Future.

https://codereview.chromium.org/1870543004/diff/1/lib/src/lazy_stream.dart
File lib/src/lazy_stream.dart (right):

https://codereview.chromium.org/1870543004/diff/1/lib/src/lazy_stream.dart#ne...
lib/src/lazy_stream.dart:47: }));
I still don't like having an async function producing a stream - a
Future<Stream>.
The callback should be using a StreamCompleter instead and just return a Stream.

https://codereview.chromium.org/1870543004/diff/1/lib/src/typed/stream.dart
File lib/src/typed/stream.dart (right):

https://codereview.chromium.org/1870543004/diff/1/lib/src/typed/stream.dart#n...
lib/src/typed/stream.dart:52: Future drain([futureValue]) =>
_stream.drain(futureValue);
I'm guessing this could just be:
  Future<S> drain<S>([S futureValue]) => _stream.drain<S>(futureValue);
The Future result of drain is primarily there to capture errors during shutdown,
and the futureValue defaults to null, so the result future is really just typed
by the futureValue and nothing else.

https://codereview.chromium.org/1870543004/diff/1/lib/src/typed/stream.dart#n...
lib/src/typed/stream.dart:133: UnaryFunction/*<dynamic, S>*/
_validateType/*<S>*/(
_validateParameterType?

https://codereview.chromium.org/1870543004/diff/1/lib/src/typed/stream_subscr...
File lib/src/typed/stream_subscription.dart (right):

https://codereview.chromium.org/1870543004/diff/1/lib/src/typed/stream_subscr...
lib/src/typed/stream_subscription.dart:35: Future asFuture([futureValue]) =>
_subscription.asFuture(futureValue);
Could be
 Future<S> asFuture<S>([S futureValue])
if you want to type it.

https://codereview.chromium.org/1870543004/diff/1/lib/src/utils.dart
File lib/src/utils.dart (right):

https://codereview.chromium.org/1870543004/diff/1/lib/src/utils.dart#newcode6
lib/src/utils.dart:6: typedef F UnaryFunction<E, F>(E argument);
We should really have a package:functions with function types and function
helpers. :)

[Lasse Reichstein Nielsen, 2016-04-08 09:20:41]

lrn@google.com changed reviewers:
+ floitsch@google.com

[nweiz, 2016-04-11 20:26:10]

Code review changes

[nweiz, 2016-04-11 20:26:16]

https://codereview.chromium.org/1870543004/diff/1/lib/src/delegate/future.dart
File lib/src/delegate/future.dart (right):

https://codereview.chromium.org/1870543004/diff/1/lib/src/delegate/future.dar...
lib/src/delegate/future.dart:16: /// Creates a wrapper that asserts the types of
the value emitted by [future].
On 2016/04/08 09:20:29, Lasse Reichstein Nielsen wrote:
> "asserts" isn't clear.
> Maybe: 
>   Creates a `Future` wrapper which throws if a value result isn't an instance
of
> `T`.
> (or similar and shorter, so it fits in a line).

Done.

https://codereview.chromium.org/1870543004/diff/1/lib/src/delegate/future.dar...
lib/src/delegate/future.dart:18: /// This soundly converts a [Future] without a
generic type to a `Future<T>`
On 2016/04/08 09:20:29, Lasse Reichstein Nielsen wrote:
> You can't have a Future without a generic type - the Future type is generic,
so
> all Future types have a type parameter, possibly "dynamic".
> 
> The function works for any Future<Type>, not just those that are dynamic. It
can
> be used to *cast* any Future type to another Future type. (We should have a
cast
> operator on Future doing this instead:
>   Future<S> cast<S>();
> and probably have that on all generic collections and boxings).
> 
> 
> 
> 

Done.

https://codereview.chromium.org/1870543004/diff/1/lib/src/delegate/future.dar...
lib/src/delegate/future.dart:19: /// by asserting that its value is an instance
of `T` whenever it's accessed.
On 2016/04/08 09:20:29, Lasse Reichstein Nielsen wrote:
> You don't "access" a future value, it's push, not pull.
> Maybe use "whenever it's provided"?

Done.

https://codereview.chromium.org/1870543004/diff/1/lib/src/delegate/future.dar...
lib/src/delegate/future.dart:20: /// If it's not, it throws a [CastError].
On 2016/04/08 09:20:29, Lasse Reichstein Nielsen wrote:
> Two "it"s referring to different things in the same sentence, the last one
needs
> to be written out.

Done.

https://codereview.chromium.org/1870543004/diff/1/lib/src/delegate/stream.dart
File lib/src/delegate/stream.dart (right):

https://codereview.chromium.org/1870543004/diff/1/lib/src/delegate/stream.dar...
lib/src/delegate/stream.dart:24: /// accessed. If they're not, it throws a
[CastError].
On 2016/04/08 09:20:29, Lasse Reichstein Nielsen wrote:
> Same comments as for Future.

Done.

https://codereview.chromium.org/1870543004/diff/1/lib/src/lazy_stream.dart
File lib/src/lazy_stream.dart (right):

https://codereview.chromium.org/1870543004/diff/1/lib/src/lazy_stream.dart#ne...
lib/src/lazy_stream.dart:47: }));
On 2016/04/08 09:20:29, Lasse Reichstein Nielsen wrote:
> I still don't like having an async function producing a stream - a
> Future<Stream>.
> The callback should be using a StreamCompleter instead and just return a
Stream.

I still think it's fine :).

https://codereview.chromium.org/1870543004/diff/1/lib/src/typed/stream.dart
File lib/src/typed/stream.dart (right):

https://codereview.chromium.org/1870543004/diff/1/lib/src/typed/stream.dart#n...
lib/src/typed/stream.dart:52: Future drain([futureValue]) =>
_stream.drain(futureValue);
On 2016/04/08 09:20:29, Lasse Reichstein Nielsen wrote:
> I'm guessing this could just be:
>   Future<S> drain<S>([S futureValue]) => _stream.drain<S>(futureValue);
> The Future result of drain is primarily there to capture errors during
shutdown,
> and the futureValue defaults to null, so the result future is really just
typed
> by the futureValue and nothing else.

That's basically what the TODO is about. Unfortunately we can't add a generic
parameter ourselves until the superclass does.

https://codereview.chromium.org/1870543004/diff/1/lib/src/typed/stream.dart#n...
lib/src/typed/stream.dart:133: UnaryFunction/*<dynamic, S>*/
_validateType/*<S>*/(
On 2016/04/08 09:20:29, Lasse Reichstein Nielsen wrote:
> _validateParameterType?

I think that's more verbose than necessary.

https://codereview.chromium.org/1870543004/diff/1/lib/src/typed/stream_subscr...
File lib/src/typed/stream_subscription.dart (right):

https://codereview.chromium.org/1870543004/diff/1/lib/src/typed/stream_subscr...
lib/src/typed/stream_subscription.dart:35: Future asFuture([futureValue]) =>
_subscription.asFuture(futureValue);
On 2016/04/08 09:20:29, Lasse Reichstein Nielsen wrote:
> Could be
>  Future<S> asFuture<S>([S futureValue])
> if you want to type it.

Strong mode won't let us add typing that's not there in the superclass. And this
type isn't public anyway, so it wouldn't do much good.

https://codereview.chromium.org/1870543004/diff/1/lib/src/utils.dart
File lib/src/utils.dart (right):

https://codereview.chromium.org/1870543004/diff/1/lib/src/utils.dart#newcode6
lib/src/utils.dart:6: typedef F UnaryFunction<E, F>(E argument);
On 2016/04/08 09:20:29, Lasse Reichstein Nielsen wrote:
> We should really have a package:functions with function types and function
> helpers. :)

I agree!

[floitsch, 2016-04-12 20:03:08]

LGTM.

https://codereview.chromium.org/1870543004/diff/20001/lib/src/delegate/event_...
File lib/src/delegate/event_sink.dart (right):

https://codereview.chromium.org/1870543004/diff/20001/lib/src/delegate/event_...
lib/src/delegate/event_sink.dart:26: sink is EventSink/*<T>*/ ? sink : new
DelegatingEventSink._(sink);
new DelegatingEventSink<T>

https://codereview.chromium.org/1870543004/diff/20001/lib/src/delegate/future...
File lib/src/delegate/future.dart (left):

https://codereview.chromium.org/1870543004/diff/20001/lib/src/delegate/future...
lib/src/delegate/future.dart:24: Future<T> timeout(Duration timeLimit, {void
onTimeout()}) =>
why this change? The value of `onTimeout()` should never be used.

https://codereview.chromium.org/1870543004/diff/20001/lib/src/delegate/future...
File lib/src/delegate/future.dart (right):

https://codereview.chromium.org/1870543004/diff/20001/lib/src/delegate/future...
lib/src/delegate/future.dart:20: /// original generic type, by asserting that
its value is an instance of `T`
The text for this one and the EventSink should be similar. From what I can see
they are doing the same thing.

https://codereview.chromium.org/1870543004/diff/20001/lib/src/lazy_stream.dart
File lib/src/lazy_stream.dart (right):

https://codereview.chromium.org/1870543004/diff/20001/lib/src/lazy_stream.dar...
lib/src/lazy_stream.dart:15: /// produce a `Stream`.
Why would it be expensive to create a Stream?
As long as no one is listening there should be no work.

Should we deprecate this class?

[Lasse Reichstein Nielsen, 2016-04-12 20:18:02]

https://codereview.chromium.org/1870543004/diff/20001/lib/src/lazy_stream.dart
File lib/src/lazy_stream.dart (right):

https://codereview.chromium.org/1870543004/diff/20001/lib/src/lazy_stream.dar...
lib/src/lazy_stream.dart:15: /// produce a `Stream`.
I think an example would be an API that provide the content of an XMLHttpRequest
call. The HTMLHttpRequest body is itself a stream, but there is an overhead (and
async delay) required in order to get access to it. This is an easy way to write
a stream that provides the same events as the request body, without doing the
initialization before you actually need it, and still return a Stream object
synchronously.

It's equivalent to a StreamCompleter with an onListen callback that creates the
stream and completes the completer.

[nweiz, 2016-04-12 20:41:20]

https://codereview.chromium.org/1870543004/diff/20001/lib/src/delegate/event_...
File lib/src/delegate/event_sink.dart (right):

https://codereview.chromium.org/1870543004/diff/20001/lib/src/delegate/event_...
lib/src/delegate/event_sink.dart:26: sink is EventSink/*<T>*/ ? sink : new
DelegatingEventSink._(sink);
On 2016/04/12 20:03:07, floitsch wrote:
> new DelegatingEventSink<T>

This is inferred from the return type.

https://codereview.chromium.org/1870543004/diff/20001/lib/src/delegate/future...
File lib/src/delegate/future.dart (left):

https://codereview.chromium.org/1870543004/diff/20001/lib/src/delegate/future...
lib/src/delegate/future.dart:24: Future<T> timeout(Duration timeLimit, {void
onTimeout()}) =>
On 2016/04/12 20:03:08, floitsch wrote:
> why this change? The value of `onTimeout()` should never be used.

This matches dart:async's declaration. The dart:async docs say:

> If this future does not complete before timeLimit has passed, the
> onTimeout action is executed instead, and its result (whether it
> returns or throws) is used as the result of the returned future.
> The onTimeout function must return a T or a Future<T>.

https://codereview.chromium.org/1870543004/diff/20001/lib/src/delegate/future...
File lib/src/delegate/future.dart (right):

https://codereview.chromium.org/1870543004/diff/20001/lib/src/delegate/future...
lib/src/delegate/future.dart:20: /// original generic type, by asserting that
its value is an instance of `T`
On 2016/04/12 20:03:08, floitsch wrote:
> The text for this one and the EventSink should be similar. From what I can see
> they are doing the same thing.

I think it's pretty different. EventSink doesn't ever pass a T to its callers,
so it doesn't make sense to talk about "its value". Instead, it adds additional
assertions about what the user passes in to the sink.

[nweiz, 2016-04-12 20:42:16]

Description was changed from

==========
Add typed wrapper functions to delegate classes.

These mirror the methods in the the collection package, and serve a
similar purpose of safely casting generic objects when the user is
confident that the actual object's values are more specific than the
static type.

R=lrn@google.com
==========

to

==========
Add typed wrapper functions to delegate classes.

These mirror the methods in the the collection package, and serve a
similar purpose of safely casting generic objects when the user is
confident that the actual object's values are more specific than the
static type.

R=floitsch@google.com, lrn@google.com

Committed:
https://github.com/dart-lang/async/commit/641e1954b4c915611e5ea9d9ab871748443...
==========

[nweiz, 2016-04-12 20:42:19]

Committed patchset #2 (id:20001) manually as
641e1954b4c915611e5ea9d9ab871748443253ff (presubmit successful).

[floitsch, 2016-04-14 13:59:45]

https://codereview.chromium.org/1870543004/diff/20001/lib/src/delegate/event_...
File lib/src/delegate/event_sink.dart (right):

https://codereview.chromium.org/1870543004/diff/20001/lib/src/delegate/event_...
lib/src/delegate/event_sink.dart:26: sink is EventSink/*<T>*/ ? sink : new
DelegatingEventSink._(sink);
On 2016/04/12 20:41:19, nweiz wrote:
> On 2016/04/12 20:03:07, floitsch wrote:
> > new DelegatingEventSink<T>
> 
> This is inferred from the return type.

Maybe, but that's non-intuitive, and it shouldn't cost to add the 'T'.

[nweiz, 2016-04-14 19:27:16]

https://codereview.chromium.org/1870543004/diff/20001/lib/src/delegate/event_...
File lib/src/delegate/event_sink.dart (right):

https://codereview.chromium.org/1870543004/diff/20001/lib/src/delegate/event_...
lib/src/delegate/event_sink.dart:26: sink is EventSink/*<T>*/ ? sink : new
DelegatingEventSink._(sink);
On 2016/04/14 13:59:45, floitsch wrote:
> On 2016/04/12 20:41:19, nweiz wrote:
> > On 2016/04/12 20:03:07, floitsch wrote:
> > > new DelegatingEventSink<T>
> > 
> > This is inferred from the return type.
> 
> Maybe, but that's non-intuitive, and it shouldn't cost to add the 'T'.

I think it's pretty intuitive. It's right there in the return type, which is
only one line away.