Add a StreamSinkTransformer class.

Add a StreamSinkTransformer class.

This makes it easier to programmatically manipulate sinks. In
particular, StreamSinkTransformer.fromStreamTransformer makes it easy to
use existing encoders and codecs when emitting data.

R=lrn@google.com

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

[Lasse Reichstein Nielsen, 2016-01-07 07:37:02]

https://codereview.chromium.org/1566603002/diff/20001/lib/src/stream_sink_tra...
File lib/src/stream_sink_transformer.dart (right):

https://codereview.chromium.org/1566603002/diff/20001/lib/src/stream_sink_tra...
lib/src/stream_sink_transformer.dart:20: /// It's good practice to write
transformers that can be used multiple times.
Not sure what this line says?
I *think* it means that you should be able to call the bind function multiple
times on the same transformer. If so, I would drop the "good practice" and say
"must".

https://codereview.chromium.org/1566603002/diff/20001/lib/src/stream_sink_tra...
lib/src/stream_sink_transformer.dart:21: abstract class StreamSinkTransformer<S,
T> {
Do we really need a class?
It has exactly one instance method, so I think
https://www.dartlang.org/effective-dart/design/#avoid-defining-a-one-member-a...
applies.

That is:

  typedef StreamSink<S> StreamSinkTransformer<S, T>(StreamSink<T> sink);

then have functions returning such a function instead of constructors returning
instances.
You can keep the implementation classes and rename "bind" to "call".

https://codereview.chromium.org/1566603002/diff/20001/lib/src/stream_sink_tra...
lib/src/stream_sink_transformer.dart:36: /// they're passed are forwarded to the
inner sink.
Specify what happens if a handler is omitted (it should default to just passing
the value through).

https://codereview.chromium.org/1566603002/diff/20001/lib/src/stream_sink_tra...
File lib/src/stream_sink_transformer/handler_transformer.dart (right):

https://codereview.chromium.org/1566603002/diff/20001/lib/src/stream_sink_tra...
lib/src/stream_sink_transformer/handler_transformer.dart:59: _inner.add(event as
T);
This does a type check, even in production mode.
Instead I would do:

  var temp = event;
  _inner.add(temp);

This will still fail in checked mode if _inner is really a StreamSink<T>, but
won't do extra type checks in production mode.

https://codereview.chromium.org/1566603002/diff/20001/lib/src/stream_sink_tra...
lib/src/stream_sink_transformer/handler_transformer.dart:78: handleDone: (sink)
=> sink.close())));
Consider having a static/top-level helper function for the handleDone callback.
It looks pretty generic, and it avoids creating a new closure each time you do
an addStream.

[nweiz, 2016-01-07 21:32:15]

Code review changes

[nweiz, 2016-01-07 21:32:26]

https://codereview.chromium.org/1566603002/diff/20001/lib/src/stream_sink_tra...
File lib/src/stream_sink_transformer.dart (right):

https://codereview.chromium.org/1566603002/diff/20001/lib/src/stream_sink_tra...
lib/src/stream_sink_transformer.dart:20: /// It's good practice to write
transformers that can be used multiple times.
On 2016/01/07 07:37:01, Lasse Reichstein Nielsen wrote:
> Not sure what this line says?
> I *think* it means that you should be able to call the bind function multiple
> times on the same transformer. If so, I would drop the "good practice" and say
> "must".

Done.

I copied this line almost verbatim from the StreamTransformer docs, so those
should probably get updated as well.

https://codereview.chromium.org/1566603002/diff/20001/lib/src/stream_sink_tra...
lib/src/stream_sink_transformer.dart:21: abstract class StreamSinkTransformer<S,
T> {
On 2016/01/07 07:37:01, Lasse Reichstein Nielsen wrote:
> Do we really need a class?
> It has exactly one instance method, so I think
>
https://www.dartlang.org/effective-dart/design/#avoid-defining-a-one-member-a...
> applies.
> 
> That is:
> 
>   typedef StreamSink<S> StreamSinkTransformer<S, T>(StreamSink<T> sink);
> 
> then have functions returning such a function instead of constructors
returning
> instances.

My biggest worry with that is how inconsistent it is with StreamTransformer.
Even if we consider the core API design a mistake in retrospect, I think it's
really useful to make core-adjacent packages like this very closely mirror the
core design. Users tend to be much more familiar with the core than with
adjacent packages, *and* they expect those packages to feel like "extra core
libraries", so any inconsistencies have an outsized effect on their experience.

It also has ripple effects on APIs that use this. With StreamTransformer,
there's a strong pattern both in and out of core of const-declared transformers,
transformers with constructor parameters, and other patterns that don't work
with a typedef'd function.

> You can keep the implementation classes and rename "bind" to "call".

Unfortunately, classes can't implement function types, even if they could be
called like those functions.

https://codereview.chromium.org/1566603002/diff/20001/lib/src/stream_sink_tra...
lib/src/stream_sink_transformer.dart:36: /// they're passed are forwarded to the
inner sink.
On 2016/01/07 07:37:01, Lasse Reichstein Nielsen wrote:
> Specify what happens if a handler is omitted (it should default to just
passing
> the value through).

Done.

https://codereview.chromium.org/1566603002/diff/20001/lib/src/stream_sink_tra...
File lib/src/stream_sink_transformer/handler_transformer.dart (right):

https://codereview.chromium.org/1566603002/diff/20001/lib/src/stream_sink_tra...
lib/src/stream_sink_transformer/handler_transformer.dart:59: _inner.add(event as
T);
On 2016/01/07 07:37:02, Lasse Reichstein Nielsen wrote:
> This does a type check, even in production mode.
> Instead I would do:
> 
>   var temp = event;
>   _inner.add(temp);
> 
> This will still fail in checked mode if _inner is really a StreamSink<T>, but
> won't do extra type checks in production mode.

Done.

https://codereview.chromium.org/1566603002/diff/20001/lib/src/stream_sink_tra...
lib/src/stream_sink_transformer/handler_transformer.dart:78: handleDone: (sink)
=> sink.close())));
On 2016/01/07 07:37:02, Lasse Reichstein Nielsen wrote:
> Consider having a static/top-level helper function for the handleDone
callback.
> It looks pretty generic, and it avoids creating a new closure each time you do
> an addStream.

It's easy to mechanically tell that this doesn't actually close over
anything—I'd rather keep the source neat and rely on the implementations to pull
it out if they decide it's worthwhile.

[Lasse Reichstein Nielsen, 2016-01-08 07:23:06]

lgtm

https://codereview.chromium.org/1566603002/diff/20001/lib/src/stream_sink_tra...
File lib/src/stream_sink_transformer.dart (right):

https://codereview.chromium.org/1566603002/diff/20001/lib/src/stream_sink_tra...
lib/src/stream_sink_transformer.dart:20: /// It's good practice to write
transformers that can be used multiple times.
On 2016/01/07 21:32:26, nweiz wrote:

> I copied this line almost verbatim from the StreamTransformer docs, so those
> should probably get updated as well.

Good point, I'll look at that.

https://codereview.chromium.org/1566603002/diff/20001/lib/src/stream_sink_tra...
lib/src/stream_sink_transformer.dart:21: abstract class StreamSinkTransformer<S,
T> {
On 2016/01/07 21:32:26, nweiz wrote:
> On 2016/01/07 07:37:01, Lasse Reichstein Nielsen wrote:
> > Do we really need a class?
> > It has exactly one instance method, so I think
> >
>
https://www.dartlang.org/effective-dart/design/#avoid-defining-a-one-member-a...
> > applies.
> > 
> > That is:
> > 
> >   typedef StreamSink<S> StreamSinkTransformer<S, T>(StreamSink<T> sink);
> > 
> > then have functions returning such a function instead of constructors
> returning
> > instances.
> 
> My biggest worry with that is how inconsistent it is with StreamTransformer.

True. StreamTransformer is used on other classes too (like Encoder), but
it's also used as stand-alone, so there is precedent.

> Even if we consider the core API design a mistake in retrospect, 

I would consider it that.

> I think it's
> really useful to make core-adjacent packages like this very closely mirror the
> core design. Users tend to be much more familiar with the core than with
> adjacent packages, *and* they expect those packages to feel like "extra core
> libraries", so any inconsistencies have an outsized effect on their
experience.

That's a valid argument. Keep the class.

> 
> It also has ripple effects on APIs that use this. With StreamTransformer,
> there's a strong pattern both in and out of core of const-declared
transformers,
> transformers with constructor parameters, and other patterns that don't work
> with a typedef'd function.

They would, if they had a call method.

> 
> > You can keep the implementation classes and rename "bind" to "call".
> 
> Unfortunately, classes can't implement function types, even if they could be
> called like those functions.

Actually, they can and do. If a class has a call method, it also has the
function type that the call method has.


class C {
  int call(int x) => 42;
}

typedef int IntFunc(int _);

main() {
  print(new C() is IntFunc);  // true!
}

https://codereview.chromium.org/1566603002/diff/20001/lib/src/stream_sink_tra...
File lib/src/stream_sink_transformer/handler_transformer.dart (right):

https://codereview.chromium.org/1566603002/diff/20001/lib/src/stream_sink_tra...
lib/src/stream_sink_transformer/handler_transformer.dart:78: handleDone: (sink)
=> sink.close())));
The implementation will not avoid creating a new function instance for every
call, even if it decides to use the same function code. Using a static function
means that there is only one instance.
(Also, I personally like to be able to see that this is a generic function that
is always the same, without having to deduce it from reading the code and seeing
that it doesn't actually use anything from the context - it confuses me to see a
local expression when the function doesn't have to be local).

https://codereview.chromium.org/1566603002/diff/40001/lib/src/stream_sink_tra...
File lib/src/stream_sink_transformer.dart (right):

https://codereview.chromium.org/1566603002/diff/40001/lib/src/stream_sink_tra...
lib/src/stream_sink_transformer.dart:20: /// Transformers must be able to be
used multiple times.
-> Transformers must be able to have `bind` called multiple times.

(calling bind is the only use, but let's be explicit).

https://codereview.chromium.org/1566603002/diff/40001/lib/src/stream_sink_tra...
File lib/src/stream_sink_transformer/handler_transformer.dart (right):

https://codereview.chromium.org/1566603002/diff/40001/lib/src/stream_sink_tra...
lib/src/stream_sink_transformer/handler_transformer.dart:62: // an extra branch
in unchecked mode.
branch -> check ?

[nweiz, 2016-01-11 21:05:47]

Code review changes

[nweiz, 2016-01-11 21:06:14]

https://codereview.chromium.org/1566603002/diff/20001/lib/src/stream_sink_tra...
File lib/src/stream_sink_transformer/handler_transformer.dart (right):

https://codereview.chromium.org/1566603002/diff/20001/lib/src/stream_sink_tra...
lib/src/stream_sink_transformer/handler_transformer.dart:78: handleDone: (sink)
=> sink.close())));
On 2016/01/08 07:23:06, Lasse Reichstein Nielsen wrote:
> The implementation will not avoid creating a new function instance for every
> call, even if it decides to use the same function code. Using a static
function
> means that there is only one instance.
> (Also, I personally like to be able to see that this is a generic function
that
> is always the same, without having to deduce it from reading the code and
seeing
> that it doesn't actually use anything from the context - it confuses me to see
a
> local expression when the function doesn't have to be local).

Done.

https://codereview.chromium.org/1566603002/diff/40001/lib/src/stream_sink_tra...
File lib/src/stream_sink_transformer.dart (right):

https://codereview.chromium.org/1566603002/diff/40001/lib/src/stream_sink_tra...
lib/src/stream_sink_transformer.dart:20: /// Transformers must be able to be
used multiple times.
On 2016/01/08 07:23:06, Lasse Reichstein Nielsen wrote:
> -> Transformers must be able to have `bind` called multiple times.
> 
> (calling bind is the only use, but let's be explicit).

Done.

https://codereview.chromium.org/1566603002/diff/40001/lib/src/stream_sink_tra...
File lib/src/stream_sink_transformer/handler_transformer.dart (right):

https://codereview.chromium.org/1566603002/diff/40001/lib/src/stream_sink_tra...
lib/src/stream_sink_transformer/handler_transformer.dart:62: // an extra branch
in unchecked mode.
On 2016/01/08 07:23:06, Lasse Reichstein Nielsen wrote:
> branch -> check ?

Done.

[nweiz, 2016-01-11 21:06:23]

Description was changed from

==========
Add a StreamSinkTransformer class.

This makes it easier to programmatically manipulate sinks. In
particular, StreamSinkTransformer.fromStreamTransformer makes it easy to
use existing encoders and codecs when emitting data.

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

to

==========
Add a StreamSinkTransformer class.

This makes it easier to programmatically manipulate sinks. In
particular, StreamSinkTransformer.fromStreamTransformer makes it easy to
use existing encoders and codecs when emitting data.

R=lrn@google.com

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

[nweiz, 2016-01-11 21:06:24]

Committed patchset #4 (id:60001) manually as
cf8815ea178e54a00980bba618c4a236f75f014d (presubmit successful).