Add a StreamSinkCompleter class.

Add a StreamSinkCompleter class.

R=lrn@google.com

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

[Lasse Reichstein Nielsen, 2016-01-21 07:28:47]

lgtm

https://codereview.chromium.org/1616543002/diff/40001/lib/src/stream_sink_com...
File lib/src/stream_sink_completer.dart (right):

https://codereview.chromium.org/1616543002/diff/40001/lib/src/stream_sink_com...
lib/src/stream_sink_completer.dart:19: class StreamSinkCompleter<T> {
I don't think I like "completer" for this particular use.
How about "StreamSinkBuffer" or "DelayedStreamSink"?
(Or something better!)

I think we have used Completer in other cases, but in hindsight I'm not sure
that was a good idea - it's a fairly nondescriptive name and Future got the
primary use of it. I don't think this class is close enough to what the future
completer does. I think the difference is that Completer and StreamCompleter
will "complete" with the incoming result, while this class lets you set the
output - the result isn't "completed" afterwards, it's just getting started.

https://codereview.chromium.org/1616543002/diff/40001/lib/src/stream_sink_com...
lib/src/stream_sink_completer.dart:23: /// sink will be forwarded to the
destination.
Do you want to promise how the events are forwarded (as timer events, as
microtasks, or all in one go?).
Probably not - it's safer to not say anything, but it will mean that you can't
change the timing later because people will depend on the actual behavior if you
don't explicitly say that the behavior is not promised.

(I *really* want to make stream-controllers deliver their buffered events using
Timer(0) instead of scheduleMicrotask, but that's probably a breaking change at
this point).

https://codereview.chromium.org/1616543002/diff/40001/lib/src/stream_sink_com...
lib/src/stream_sink_completer.dart:28: final _sink = new _CompleterSink<T>();
Just make it one field:
  final StreamSink<T> sink = new _CompleterSink<T>();

You may need to assign it to a local variable for typing when using:
  _CompleterSink sink = _sink;
  ...
since _sink is dynamic.

https://codereview.chromium.org/1616543002/diff/40001/lib/src/stream_sink_com...
lib/src/stream_sink_completer.dart:30: /// Set a sink as the destination for
events from the [StreamSinkCompleter]'s
Set -> Sets

https://codereview.chromium.org/1616543002/diff/40001/lib/src/stream_sink_com...
lib/src/stream_sink_completer.dart:42: void setDestinationSink(StreamSink<T>
destinationSink) {
Make this a setter.

https://codereview.chromium.org/1616543002/diff/40001/lib/src/stream_sink_com...
lib/src/stream_sink_completer.dart:46:
_sink._setDestinationSink(destinationSink);
Can you find another name than this (in particular, one that doesn't start with
"set")?
I'm not coming up with any, but this name looks like something which should be a
setter.
If you stick with "Completer" in the class name, maybe name it "completeSink"
(which sounds bad, so maybe it isn't a completer :)

https://codereview.chromium.org/1616543002/diff/40001/lib/src/stream_sink_com...
lib/src/stream_sink_completer.dart:71: bool get _sendToSink => _controller ==
null && _destinationSink != null;
Name sounds imperative. Maybe "_hasDirectSink" or something less awkward?

If you can't find a way to prefix an "is", "has", "will" or similar, maybe just
change to "_sendsToSink".

https://codereview.chromium.org/1616543002/diff/40001/lib/src/stream_sink_com...
lib/src/stream_sink_completer.dart:119: if (_controller == null) _controller =
new StreamController<T>(sync: true);
Persionally, I'd drop the type parameter on the completely internal controller.
The user will see neither the controller nor the stream, so it doesn't matter,
and we have type parameters on the add method and setDestinationSink.

Completely personal preference.

https://codereview.chromium.org/1616543002/diff/40001/test/stream_sink_comple...
File test/stream_sink_completer_test.dart (right):

https://codereview.chromium.org/1616543002/diff/40001/test/stream_sink_comple...
test/stream_sink_completer_test.dart:101: await flushMicrotasks();
I'd wait for a timer event here, to be sure all microtasks are done.
await new Future((){}); should do it, but you can probably give it a more
readable name.

https://codereview.chromium.org/1616543002/diff/40001/test/stream_sink_comple...
test/stream_sink_completer_test.dart:232: });
Could you make more tests with the "done" getter:
- Cover all six orderings of add event, read done, setDestinationSink
- make the sink's onDone return something recognizable, so yo can see that the
onDone gets the correct result in all cases.

[nweiz, 2016-01-21 20:55:40]

Code review changes

[nweiz, 2016-01-21 20:59:05]

https://codereview.chromium.org/1616543002/diff/40001/lib/src/stream_sink_com...
File lib/src/stream_sink_completer.dart (right):

https://codereview.chromium.org/1616543002/diff/40001/lib/src/stream_sink_com...
lib/src/stream_sink_completer.dart:19: class StreamSinkCompleter<T> {
On 2016/01/21 07:28:47, Lasse Reichstein Nielsen wrote:
> I don't think I like "completer" for this particular use.
> How about "StreamSinkBuffer" or "DelayedStreamSink"?
> (Or something better!)
> 
> I think we have used Completer in other cases, but in hindsight I'm not sure
> that was a good idea - it's a fairly nondescriptive name and Future got the
> primary use of it. I don't think this class is close enough to what the future
> completer does. I think the difference is that Completer and StreamCompleter
> will "complete" with the incoming result, while this class lets you set the
> output - the result isn't "completed" afterwards, it's just getting started.

I really think consistency with parallel APIs is important here. I intend to use
this alongside a similar use of StreamCompleter, and I suspect many uses will be
similar. Having the names be similar helps emphasize their parallel purpose.

In general, I also think that broadening an existing term to cover variations of
the concept is often better than creating a new term. It's easier for users to
understand "Completer" to mean "supplying a concrete object after the fact" than
it is for them to learn a use of "Buffered" or "Delayed" to describe a class
whose usage isn't closely related to any other use of those terms in Dart's
libraries.

https://codereview.chromium.org/1616543002/diff/40001/lib/src/stream_sink_com...
lib/src/stream_sink_completer.dart:23: /// sink will be forwarded to the
destination.
On 2016/01/21 07:28:47, Lasse Reichstein Nielsen wrote:
> Do you want to promise how the events are forwarded (as timer events, as
> microtasks, or all in one go?).
> Probably not - it's safer to not say anything, but it will mean that you can't
> change the timing later because people will depend on the actual behavior if
you
> don't explicitly say that the behavior is not promised.

I don't think we can provide any stronger guarantees than [StreamController]
does already. Since it's unspecified, I think we have to leave this unspecified
as well.

> (I *really* want to make stream-controllers deliver their buffered events
using
> Timer(0) instead of scheduleMicrotask, but that's probably a breaking change
at
> this point).

Why, out of curiosity? That seems inconsistent with Futures.

https://codereview.chromium.org/1616543002/diff/40001/lib/src/stream_sink_com...
lib/src/stream_sink_completer.dart:28: final _sink = new _CompleterSink<T>();
On 2016/01/21 07:28:47, Lasse Reichstein Nielsen wrote:
> Just make it one field:
>   final StreamSink<T> sink = new _CompleterSink<T>();
> 
> You may need to assign it to a local variable for typing when using:
>   _CompleterSink sink = _sink;
>   ...
> since _sink is dynamic.

Done.

https://codereview.chromium.org/1616543002/diff/40001/lib/src/stream_sink_com...
lib/src/stream_sink_completer.dart:30: /// Set a sink as the destination for
events from the [StreamSinkCompleter]'s
On 2016/01/21 07:28:47, Lasse Reichstein Nielsen wrote:
> Set -> Sets

Done.

https://codereview.chromium.org/1616543002/diff/40001/lib/src/stream_sink_com...
lib/src/stream_sink_completer.dart:42: void setDestinationSink(StreamSink<T>
destinationSink) {
On 2016/01/21 07:28:47, Lasse Reichstein Nielsen wrote:
> Make this a setter.

That's inconsistent with StreamCompleter (and Completer for that matter). Also,
the design guide says not to use a setter without a corresponding getter
(https://www.dartlang.org/effective-dart/design/#dont-define-a-setter-without-...),
and I don't think a getter makes sense here.

https://codereview.chromium.org/1616543002/diff/40001/lib/src/stream_sink_com...
lib/src/stream_sink_completer.dart:46:
_sink._setDestinationSink(destinationSink);
On 2016/01/21 07:28:47, Lasse Reichstein Nielsen wrote:
> Can you find another name than this (in particular, one that doesn't start
with
> "set")?
> I'm not coming up with any, but this name looks like something which should be
a
> setter.
> If you stick with "Completer" in the class name, maybe name it "completeSink"
> (which sounds bad, so maybe it isn't a completer :)

This is consistent with StreamCompleter (which you wrote ;) ). I think it's okay
to have methods start with "set" when the setting is meant to have side effects
rather than making a property accessible.

That said, if you feel strongly that "set" is wrong, I can send out a follow-up
CL that changes it to "complete" (which I don't mind much) and deprecates
StreamCompleter.setStream() in favor of StreamCompleter.completeStream().

https://codereview.chromium.org/1616543002/diff/40001/lib/src/stream_sink_com...
lib/src/stream_sink_completer.dart:71: bool get _sendToSink => _controller ==
null && _destinationSink != null;
On 2016/01/21 07:28:47, Lasse Reichstein Nielsen wrote:
> Name sounds imperative. Maybe "_hasDirectSink" or something less awkward?
> 
> If you can't find a way to prefix an "is", "has", "will" or similar, maybe
just
> change to "_sendsToSink".

Done.

https://codereview.chromium.org/1616543002/diff/40001/lib/src/stream_sink_com...
lib/src/stream_sink_completer.dart:119: if (_controller == null) _controller =
new StreamController<T>(sync: true);
On 2016/01/21 07:28:47, Lasse Reichstein Nielsen wrote:
> Persionally, I'd drop the type parameter on the completely internal
controller.
> The user will see neither the controller nor the stream, so it doesn't matter,
> and we have type parameters on the add method and setDestinationSink.
> 
> Completely personal preference.

Done.

https://codereview.chromium.org/1616543002/diff/40001/test/stream_sink_comple...
File test/stream_sink_completer_test.dart (right):

https://codereview.chromium.org/1616543002/diff/40001/test/stream_sink_comple...
test/stream_sink_completer_test.dart:101: await flushMicrotasks();
On 2016/01/21 07:28:47, Lasse Reichstein Nielsen wrote:
> I'd wait for a timer event here, to be sure all microtasks are done.
> await new Future((){}); should do it, but you can probably give it a more
> readable name.

That's exactly what flushMicrotasks() does, actually :).

https://codereview.chromium.org/1616543002/diff/40001/test/stream_sink_comple...
test/stream_sink_completer_test.dart:232: });
On 2016/01/21 07:28:47, Lasse Reichstein Nielsen wrote:
> Could you make more tests with the "done" getter:
> - Cover all six orderings of add event, read done, setDestinationSink

Done. Four of these were already being tested, so I added explicit tests for the
last two.

> - make the sink's onDone return something recognizable, so yo can see that the
> onDone gets the correct result in all cases.

I don't like checking the value, because the "done" future really shouldn't have
a value anyway (it would be a Future<void> if we had a way of representing
that). But I do check that it blocks as long as the returned future does, and I
check that any internal errors thrown are piped correctly.

[nweiz, 2016-01-21 20:59:28]

Description was changed from

==========
Add a StreamSinkCompleter class.

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

to

==========
Add a StreamSinkCompleter class.

R=lrn@google.com

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

[nweiz, 2016-01-21 20:59:29]

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

[Lasse Reichstein Nielsen, 2016-01-22 12:49:27]

https://codereview.chromium.org/1616543002/diff/40001/lib/src/stream_sink_com...
File lib/src/stream_sink_completer.dart (right):

https://codereview.chromium.org/1616543002/diff/40001/lib/src/stream_sink_com...
lib/src/stream_sink_completer.dart:19: class StreamSinkCompleter<T> {
Acknowledged. 
It doesn't fit my mental model of what "completing" means (putting in the
*result*), but I guess it's consistent in its own way.

https://codereview.chromium.org/1616543002/diff/40001/lib/src/stream_sink_com...
lib/src/stream_sink_completer.dart:23: /// sink will be forwarded to the
destination.
On 2016/01/21 20:59:05, nweiz wrote:

> I don't think we can provide any stronger guarantees than [StreamController]
> does already. Since it's unspecified, I think we have to leave this
unspecified
> as well.

That's actually bad - leaving something unspecified implicitly means that users
can only depend on what it actually does. If we say explicitly that there are no
promises, then users won't be able to (reasonably) depend on the actual
behavior. 

> 
> > (I *really* want to make stream-controllers deliver their buffered events
> using
> > Timer(0) instead of scheduleMicrotask, but that's probably a breaking change
> at
> > this point).
> 
> Why, out of curiosity? That seems inconsistent with Futures.

The thing is that if a stream has a lot of buffered events, the code handling
them will become completely interleaved. If instead it waited until the
microtask queue is empty to release the next event, then the event handling code
will be nicely separated into chunks related to each event.

The total amount of work would be the same, I just like the ordering better when
the microtasks triggered by an event gets to complete before the next event is
fired.

https://codereview.chromium.org/1616543002/diff/40001/lib/src/stream_sink_com...
lib/src/stream_sink_completer.dart:42: void setDestinationSink(StreamSink<T>
destinationSink) {
Argument accepted.

It is probably better as a method than a setter. Especially when it can only set
once.

https://codereview.chromium.org/1616543002/diff/40001/lib/src/stream_sink_com...
lib/src/stream_sink_completer.dart:46:
_sink._setDestinationSink(destinationSink);
The problem is that "set" isn't wrong. It just looks like it should have been a
setter. But I think you are right that this is better than the alternatives.