Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
196423021:
Move _StreamSinkImpl from dart:io to dart:async as StreamSinkAdapter. (Closed)
|
Created:
6 years, 9 months ago by Anders Johnsen Modified:
6 years, 1 month ago CC:
reviews_dartlang.org Visibility:
Public. |
DescriptionMove _StreamSinkImpl from dart:io to dart:async as StreamSinkAdapter.
BUG=https://code.google.com/p/dart/issues/detail?id=17293
Patch Set 1 #
Total comments: 18
Patch Set 2 : Fix doc and add test.dart #
Total comments: 30
Patch Set 3 : #Patch Set 4 : http://code.google.com/p/dart/issues/detail?id=17826 #Messages
Total messages: 8 (0 generated)
DBC: Tests? Thanks for doing this.
LGTM! https://codereview.chromium.org/196423021/diff/1/sdk/lib/async/stream.dart File sdk/lib/async/stream.dart (right): https://codereview.chromium.org/196423021/diff/1/sdk/lib/async/stream.dart#ne... sdk/lib/async/stream.dart:1329: * A `StreamSinkAdapter` is an adapter implementation that can be used on a Too long first line. How about: An adapter that allows using a [StreamConsumer] as a [StreamSink]. Or perhaps: A [StreamSink] adapter for a [StreamConsumer]. (The word "adapter" should be fine, since it is a classical OO pattern). https://codereview.chromium.org/196423021/diff/1/sdk/lib/async/stream.dart#ne... sdk/lib/async/stream.dart:1333: * and will delay an [addStream] until the buffer is flushed. the buffer -> that buffer This sounds like nothing is written until you call "flush"? How about: ... until all buffered data has been forwarded to the stream consumer. https://codereview.chromium.org/196423021/diff/1/sdk/lib/async/stream.dart#ne... sdk/lib/async/stream.dart:1335: * When the `StreamSinkAdapter` is bound to a stream (through [addStream]) any When -> While https://codereview.chromium.org/196423021/diff/1/sdk/lib/async/stream.dart#ne... sdk/lib/async/stream.dart:1336: * call to the `StreamSinkAdapter` will throw a [StateError]. Is this all calls? What about "done" (which is a getter, not a method, but still...)? https://codereview.chromium.org/196423021/diff/1/sdk/lib/async/stream.dart#ne... sdk/lib/async/stream.dart:1361: void add(S data) { No _isBound check? https://codereview.chromium.org/196423021/diff/1/sdk/lib/async/stream.dart#ne... sdk/lib/async/stream.dart:1367: _controller.addError(error, stackTrace); No _isBound check? https://codereview.chromium.org/196423021/diff/1/sdk/lib/async/stream.dart#ne... sdk/lib/async/stream.dart:1394: * NOTE: This is not necessarily the same as the data being flushed by the This note is confusion. It says what this call isn't (or might not be), which is extremely vague. Is it sometimes the same? When? What is it when it is not the same? How about: NOTE: This method does not guarantee anything except that the stream consumer has received the data. It does not guarantee that the consumer has acted on the data in any way, or that the data has reached its final destination. https://codereview.chromium.org/196423021/diff/1/sdk/lib/async/stream.dart#ne... sdk/lib/async/stream.dart:1398: if (_isBound) { Consider having a helper function for this check, e.g., "_checkNotBound()". https://codereview.chromium.org/196423021/diff/1/sdk/lib/io/io_sink.dart File sdk/lib/io/io_sink.dart (right): https://codereview.chromium.org/196423021/diff/1/sdk/lib/io/io_sink.dart#newc... sdk/lib/io/io_sink.dart:15: * the [IOSink] will again be open to all calls. ... will again accept all method calls.
And tests, yes! Lots of test.
PTAL! Added test :) https://codereview.chromium.org/196423021/diff/1/sdk/lib/async/stream.dart File sdk/lib/async/stream.dart (right): https://codereview.chromium.org/196423021/diff/1/sdk/lib/async/stream.dart#ne... sdk/lib/async/stream.dart:1329: * A `StreamSinkAdapter` is an adapter implementation that can be used on a On 2014/03/18 15:34:45, Lasse Reichstein Nielsen wrote: > Too long first line. How about: > > An adapter that allows using a [StreamConsumer] as a [StreamSink]. > > Or perhaps: > > A [StreamSink] adapter for a [StreamConsumer]. > > (The word "adapter" should be fine, since it is a classical OO pattern). Done. https://codereview.chromium.org/196423021/diff/1/sdk/lib/async/stream.dart#ne... sdk/lib/async/stream.dart:1333: * and will delay an [addStream] until the buffer is flushed. On 2014/03/18 15:34:45, Lasse Reichstein Nielsen wrote: > the buffer -> that buffer > > This sounds like nothing is written until you call "flush"? > How about: > ... until all buffered data has been forwarded to the stream consumer. Done. https://codereview.chromium.org/196423021/diff/1/sdk/lib/async/stream.dart#ne... sdk/lib/async/stream.dart:1335: * When the `StreamSinkAdapter` is bound to a stream (through [addStream]) any On 2014/03/18 15:34:45, Lasse Reichstein Nielsen wrote: > When -> While Done. https://codereview.chromium.org/196423021/diff/1/sdk/lib/async/stream.dart#ne... sdk/lib/async/stream.dart:1336: * call to the `StreamSinkAdapter` will throw a [StateError]. On 2014/03/18 15:34:45, Lasse Reichstein Nielsen wrote: > Is this all calls? What about "done" (which is a getter, not a method, but > still...)? Yes, only done... :) https://codereview.chromium.org/196423021/diff/1/sdk/lib/async/stream.dart#ne... sdk/lib/async/stream.dart:1361: void add(S data) { On 2014/03/18 15:34:45, Lasse Reichstein Nielsen wrote: > No _isBound check? Is in _controller getter. https://codereview.chromium.org/196423021/diff/1/sdk/lib/async/stream.dart#ne... sdk/lib/async/stream.dart:1367: _controller.addError(error, stackTrace); On 2014/03/18 15:34:45, Lasse Reichstein Nielsen wrote: > No _isBound check? Is in _controller getter. https://codereview.chromium.org/196423021/diff/1/sdk/lib/async/stream.dart#ne... sdk/lib/async/stream.dart:1394: * NOTE: This is not necessarily the same as the data being flushed by the On 2014/03/18 15:34:45, Lasse Reichstein Nielsen wrote: > This note is confusion. It says what this call isn't (or might not be), which is > extremely vague. Is it sometimes the same? When? What is it when it is not the > same? > > How about: > NOTE: This method does not guarantee anything except that the stream consumer > has received the data. It does not guarantee that the consumer has acted on the > data in any way, or that the data has reached its final destination. Done. https://codereview.chromium.org/196423021/diff/1/sdk/lib/async/stream.dart#ne... sdk/lib/async/stream.dart:1398: if (_isBound) { On 2014/03/18 15:34:45, Lasse Reichstein Nielsen wrote: > Consider having a helper function for this check, e.g., "_checkNotBound()". Done. https://codereview.chromium.org/196423021/diff/1/sdk/lib/io/io_sink.dart File sdk/lib/io/io_sink.dart (right): https://codereview.chromium.org/196423021/diff/1/sdk/lib/io/io_sink.dart#newc... sdk/lib/io/io_sink.dart:15: * the [IOSink] will again be open to all calls. On 2014/03/18 15:34:45, Lasse Reichstein Nielsen wrote: > ... will again accept all method calls. Done.
https://codereview.chromium.org/196423021/diff/20001/sdk/lib/async/stream.dart File sdk/lib/async/stream.dart (right): https://codereview.chromium.org/196423021/diff/20001/sdk/lib/async/stream.dar... sdk/lib/async/stream.dart:1370: _checkIsBound(); Hmm, would probably sounds better as "_checkNotBound()", since being bound will fail the check. https://codereview.chromium.org/196423021/diff/20001/sdk/lib/async/stream.dar... sdk/lib/async/stream.dart:1372: if (_hasError) return done; return _doneFuture; (I generally try to avoid having public method call other public methods where possible) https://codereview.chromium.org/196423021/diff/20001/sdk/lib/async/stream.dar... sdk/lib/async/stream.dart:1387: * Returns a [Future] that completes once all buffered events is accepted by is -> has been. https://codereview.chromium.org/196423021/diff/20001/sdk/lib/async/stream.dar... sdk/lib/async/stream.dart:1390: * It's an error to call this method, while an [addStream] is incomplete. -> This method must not be called while an [addStream] is in progress. https://codereview.chromium.org/196423021/diff/20001/sdk/lib/async/stream.dar... sdk/lib/async/stream.dart:1393: * operating system. Remote this NOTE, keep the next. https://codereview.chromium.org/196423021/diff/20001/sdk/lib/async/stream.dar... sdk/lib/async/stream.dart:1401: if (_controllerInstance == null) return new Future.value(this); Why is "this" the value of the future? Is this documented anywhere? Is it used? (I would just use null). https://codereview.chromium.org/196423021/diff/20001/sdk/lib/async/stream.dar... sdk/lib/async/stream.dart:1409: }); Indentation: Unindent the previous two lines by four. https://codereview.chromium.org/196423021/diff/20001/sdk/lib/async/stream.dar... sdk/lib/async/stream.dart:1435: void _completeDone({value, error}) { This function expects one of two arguments. Make it two functions: void _completeDoneValue(value) { if (_doneCompleter == null) return; _doneCompleter.complete(value); _doneCompleter = null; } void _completeDoneError(error, stacktrace) { if (_doneCompleter == null) return; _hasError = true; _doneCompleter.completeError(error, stacktrace); _doneCompleter = null; } Then you can register it as: .then(_completeDoneValue, onError: _completeDoneError) and avoid some extra closures. https://codereview.chromium.org/196423021/diff/20001/sdk/lib/async/stream.dar... sdk/lib/async/stream.dart:1485: throw new StateError("StreamSink is bound to a stream"); ... is bound to a stream -> ... is processing an addStream call We do mention "bound" once above, but this is probably easier to understand. https://codereview.chromium.org/196423021/diff/20001/tests/lib/async/stream_s... File tests/lib/async/stream_sink_adapter_test.dart (right): https://codereview.chromium.org/196423021/diff/20001/tests/lib/async/stream_s... tests/lib/async/stream_sink_adapter_test.dart:10: class StreamConsumerImpl implements StreamConsumer { Impl isn't saying anything. How about ExpectingStreamConsumer (or ExpectStreamConsumer or TestStreamConsumer if the former sounds too pregnant). https://codereview.chromium.org/196423021/diff/20001/tests/lib/async/stream_s... tests/lib/async/stream_sink_adapter_test.dart:17: return stream.listen((event) { return stream.listen(events.add).asFuture(); https://codereview.chromium.org/196423021/diff/20001/tests/lib/async/stream_s... tests/lib/async/stream_sink_adapter_test.dart:32: Add documentation for tests: What is it testing, how can it fail? https://codereview.chromium.org/196423021/diff/20001/tests/lib/async/stream_s... tests/lib/async/stream_sink_adapter_test.dart:56: // Not valid while flush. while flush -> during flush. (or "while flushing", but I prefer the one above). https://codereview.chromium.org/196423021/diff/20001/tests/lib/async/stream_s... tests/lib/async/stream_sink_adapter_test.dart:90: }); What happens if you do "sink.add(0);" here? Does it throw? Should it throw? https://codereview.chromium.org/196423021/diff/20001/tests/lib/async/stream_s... tests/lib/async/stream_sink_adapter_test.dart:105: void main() { Add "asyncStart()" before calling the tests, and "asyncEnd();" after.
Where to take it from here? StreamSink.fromStreamConsumer - and what is the class name, as we do have a new method, `flush` ? https://codereview.chromium.org/196423021/diff/20001/sdk/lib/async/stream.dart File sdk/lib/async/stream.dart (right): https://codereview.chromium.org/196423021/diff/20001/sdk/lib/async/stream.dar... sdk/lib/async/stream.dart:1370: _checkIsBound(); On 2014/03/19 13:27:20, Lasse Reichstein Nielsen wrote: > Hmm, would probably sounds better as "_checkNotBound()", since being bound will > fail the check. Done. https://codereview.chromium.org/196423021/diff/20001/sdk/lib/async/stream.dar... sdk/lib/async/stream.dart:1372: if (_hasError) return done; On 2014/03/19 13:27:20, Lasse Reichstein Nielsen wrote: > return _doneFuture; > > (I generally try to avoid having public method call other public methods where > possible) Done. https://codereview.chromium.org/196423021/diff/20001/sdk/lib/async/stream.dar... sdk/lib/async/stream.dart:1387: * Returns a [Future] that completes once all buffered events is accepted by On 2014/03/19 13:27:20, Lasse Reichstein Nielsen wrote: > is -> has been. Done. https://codereview.chromium.org/196423021/diff/20001/sdk/lib/async/stream.dar... sdk/lib/async/stream.dart:1390: * It's an error to call this method, while an [addStream] is incomplete. On 2014/03/19 13:27:20, Lasse Reichstein Nielsen wrote: > -> This method must not be called while an [addStream] is in progress. Done. https://codereview.chromium.org/196423021/diff/20001/sdk/lib/async/stream.dar... sdk/lib/async/stream.dart:1393: * operating system. On 2014/03/19 13:27:20, Lasse Reichstein Nielsen wrote: > Remote this NOTE, keep the next. Done. https://codereview.chromium.org/196423021/diff/20001/sdk/lib/async/stream.dar... sdk/lib/async/stream.dart:1401: if (_controllerInstance == null) return new Future.value(this); On 2014/03/19 13:27:20, Lasse Reichstein Nielsen wrote: > Why is "this" the value of the future? Is this documented anywhere? Is it used? > (I would just use null). Done. https://codereview.chromium.org/196423021/diff/20001/sdk/lib/async/stream.dar... sdk/lib/async/stream.dart:1409: }); On 2014/03/19 13:27:20, Lasse Reichstein Nielsen wrote: > Indentation: Unindent the previous two lines by four. Done. https://codereview.chromium.org/196423021/diff/20001/sdk/lib/async/stream.dar... sdk/lib/async/stream.dart:1435: void _completeDone({value, error}) { On 2014/03/19 13:27:20, Lasse Reichstein Nielsen wrote: > This function expects one of two arguments. > Make it two functions: > void _completeDoneValue(value) { > if (_doneCompleter == null) return; > _doneCompleter.complete(value); > _doneCompleter = null; > } > void _completeDoneError(error, stacktrace) { > if (_doneCompleter == null) return; > _hasError = true; > _doneCompleter.completeError(error, stacktrace); > _doneCompleter = null; > } > > Then you can register it as: > .then(_completeDoneValue, onError: _completeDoneError) > and avoid some extra closures. Done. https://codereview.chromium.org/196423021/diff/20001/sdk/lib/async/stream.dar... sdk/lib/async/stream.dart:1485: throw new StateError("StreamSink is bound to a stream"); On 2014/03/19 13:27:20, Lasse Reichstein Nielsen wrote: > ... is bound to a stream -> ... is processing an addStream call > > We do mention "bound" once above, but this is probably easier to understand. Done. https://codereview.chromium.org/196423021/diff/20001/tests/lib/async/stream_s... File tests/lib/async/stream_sink_adapter_test.dart (right): https://codereview.chromium.org/196423021/diff/20001/tests/lib/async/stream_s... tests/lib/async/stream_sink_adapter_test.dart:10: class StreamConsumerImpl implements StreamConsumer { On 2014/03/19 13:27:20, Lasse Reichstein Nielsen wrote: > Impl isn't saying anything. How about ExpectingStreamConsumer (or > ExpectStreamConsumer or TestStreamConsumer if the former sounds too pregnant). Done. https://codereview.chromium.org/196423021/diff/20001/tests/lib/async/stream_s... tests/lib/async/stream_sink_adapter_test.dart:17: return stream.listen((event) { On 2014/03/19 13:27:20, Lasse Reichstein Nielsen wrote: > return stream.listen(events.add).asFuture(); > Done. https://codereview.chromium.org/196423021/diff/20001/tests/lib/async/stream_s... tests/lib/async/stream_sink_adapter_test.dart:32: On 2014/03/19 13:27:20, Lasse Reichstein Nielsen wrote: > Add documentation for tests: What is it testing, how can it fail? Done. https://codereview.chromium.org/196423021/diff/20001/tests/lib/async/stream_s... tests/lib/async/stream_sink_adapter_test.dart:56: // Not valid while flush. On 2014/03/19 13:27:20, Lasse Reichstein Nielsen wrote: > while flush -> during flush. > (or "while flushing", but I prefer the one above). Done. https://codereview.chromium.org/196423021/diff/20001/tests/lib/async/stream_s... tests/lib/async/stream_sink_adapter_test.dart:90: }); On 2014/03/19 13:27:20, Lasse Reichstein Nielsen wrote: > What happens if you do "sink.add(0);" here? Does it throw? Should it throw? Done. https://codereview.chromium.org/196423021/diff/20001/tests/lib/async/stream_s... tests/lib/async/stream_sink_adapter_test.dart:105: void main() { On 2014/03/19 13:27:20, Lasse Reichstein Nielsen wrote: > Add "asyncStart()" before calling the tests, and "asyncEnd();" after. Done.
Where'd all the files go? |
