Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2899273003:
Update stream documentation. (Closed)
|
Created:
3 years, 7 months ago by Lasse Reichstein Nielsen Modified:
3 years, 6 months ago Reviewers:
floitsch CC:
reviews_dartlang.org Target Ref:
refs/heads/master Visibility:
Public. |
DescriptionUpdate stream documentation.
Fixes #29694
BUG= http://dartbug.com/29694
R=floitsch@google.com
Committed: https://github.com/dart-lang/sdk/commit/fbe98e6bfc57fa584e5fcd04b61d1c65a8af0c88
Patch Set 1 #
Total comments: 36
Patch Set 2 : Address comments. #
Total comments: 2
Messages
Total messages: 7 (2 generated)
lrn@google.com changed reviewers: + floitsch@google.com
LGTM. thanks. https://codereview.chromium.org/2899273003/diff/1/sdk/lib/async/stream.dart File sdk/lib/async/stream.dart (right): https://codereview.chromium.org/2899273003/diff/1/sdk/lib/async/stream.dart#n... sdk/lib/async/stream.dart:17: * Each event is either a data event, also called an *element* of the stream, isn't `_element_` the better way for this? https://codereview.chromium.org/2899273003/diff/1/sdk/lib/async/stream.dart#n... sdk/lib/async/stream.dart:134: // Declare as variable holding closure instead of as a function declaration. holding a closure https://codereview.chromium.org/2899273003/diff/1/sdk/lib/async/stream.dart#n... sdk/lib/async/stream.dart:365: * error emitted on the returned stream instead. error is emitted https://codereview.chromium.org/2899273003/diff/1/sdk/lib/async/stream.dart#n... sdk/lib/async/stream.dart:376: * Transform each element of this stream into a new stream event. Transforms https://codereview.chromium.org/2899273003/diff/1/sdk/lib/async/stream.dart#n... sdk/lib/async/stream.dart:529: * Transform each element into a sequence of asynchronous events. Transforms https://codereview.chromium.org/2899273003/diff/1/sdk/lib/async/stream.dart#n... sdk/lib/async/stream.dart:531: * Returns a new stream that emits the error events and done event I think we should start with what happens to the elements. We need to say what happens to errors and done, but it should be in a separate later sentence. For example: --- Returns a new stream where each element of this stream is used to create a new stream (via [convert]). The new stream's events (elements and errors) are added individually to the returned stream. Error events and the done event of this stream are passed through, verbatim. For every element, this stream pauses its input until the new stream is done. --- Alternatively, we could adopt a more formal approach. For example: --- Returns a stream which is the result of transforming all events: - each element `e` is passed to [convert]. If this operation throws, the resulting exception is passed as error event. Otherwise, this stream is paused and all events (except done) are passed verbatim. Once the new stream is done, this stream is resumed. - errors are passed through. - done is passed through. --- I like the idea of having a nice structure, but I'm not really happy with my proposal yet. https://codereview.chromium.org/2899273003/diff/1/sdk/lib/async/stream.dart#n... sdk/lib/async/stream.dart:631: * Transform each element of this stream into a sqeuence of elements. Transforms ... sequence ... https://codereview.chromium.org/2899273003/diff/1/sdk/lib/async/stream.dart#n... sdk/lib/async/stream.dart:633: * Returns a new stream which emits the same error and done events as See above. https://codereview.chromium.org/2899273003/diff/1/sdk/lib/async/stream.dart#n... sdk/lib/async/stream.dart:651: * Pipe the events of this stream into [streamConsumer]. Pipes https://codereview.chromium.org/2899273003/diff/1/sdk/lib/async/stream.dart#n... sdk/lib/async/stream.dart:663: * If the call to [StreamConsumer.addStream] fails in some way, this "fails" -> "throws" https://codereview.chromium.org/2899273003/diff/1/sdk/lib/async/stream.dart#n... sdk/lib/async/stream.dart:682: * The [streamTransformer] can return any stream, any stream. Whether the ... Maybe mention, that transform can be used as a building block for all transforming methods, and that it is usually preferable to use existing methods if they perform the intended task. https://codereview.chromium.org/2899273003/diff/1/sdk/lib/async/stream.dart#n... sdk/lib/async/stream.dart:792: * and procssing stops. processing https://codereview.chromium.org/2899273003/diff/1/sdk/lib/async/stream.dart#n... sdk/lib/async/stream.dart:818: * Checks whether [needle] occurs in the elements provided by this stream. Returns https://codereview.chromium.org/2899273003/diff/1/sdk/lib/async/stream.dart#n... sdk/lib/async/stream.dart:1586: * Set or override the data event handler of this subscription. Set*s* or replaces ? Maybe just "replaces". After all, it even replaces a `null`. ("Override" has other meanings) Here and in the other places. https://codereview.chromium.org/2899273003/diff/1/sdk/lib/async/stream.dart#n... sdk/lib/async/stream.dart:1590: * If [handleData] is `null`, furhter elements are ignored. further https://codereview.chromium.org/2899273003/diff/1/sdk/lib/async/stream.dart#n... sdk/lib/async/stream.dart:1661: * When all previously calls to [pause] has been matched by a calls to have been matched https://codereview.chromium.org/2899273003/diff/1/sdk/lib/async/stream.dart#n... sdk/lib/async/stream.dart:1663: * the stream subscription will again emit events. subscription emits again events. (No need for future tense) https://codereview.chromium.org/2899273003/diff/1/sdk/lib/async/stream.dart#n... sdk/lib/async/stream.dart:1668: * Returns true if the [StreamSubscription] is currently paused. Whether the ... https://codereview.chromium.org/2899273003/diff/1/sdk/lib/async/stream.dart#n... sdk/lib/async/stream.dart:2072: /// The stream created by `groupBy` emits a `GroupedEvents` for each distinct key long line
https://codereview.chromium.org/2899273003/diff/1/sdk/lib/async/stream.dart File sdk/lib/async/stream.dart (right): https://codereview.chromium.org/2899273003/diff/1/sdk/lib/async/stream.dart#n... sdk/lib/async/stream.dart:17: * Each event is either a data event, also called an *element* of the stream, It's equivalent in commonmark. http://spec.commonmark.org/0.27/#emphasis-and-strong-emphasis There is a distinction between `_` and `*`, but only in some tricky cases. In the plain case of emphasizing a single word, they do exactly the same. (And I tend to read `*` as emphasis and `_` as underline, so I prefer `*`). https://codereview.chromium.org/2899273003/diff/1/sdk/lib/async/stream.dart#n... sdk/lib/async/stream.dart:134: // Declare as variable holding closure instead of as a function declaration. And more rewrite of that broken sentence. https://codereview.chromium.org/2899273003/diff/1/sdk/lib/async/stream.dart#n... sdk/lib/async/stream.dart:365: * error emitted on the returned stream instead. I think it can read without the "is", but it's probably simple with. https://codereview.chromium.org/2899273003/diff/1/sdk/lib/async/stream.dart#n... sdk/lib/async/stream.dart:376: * Transform each element of this stream into a new stream event. On 2017/05/24 13:37:50, floitsch wrote: > Transforms Done. https://codereview.chromium.org/2899273003/diff/1/sdk/lib/async/stream.dart#n... sdk/lib/async/stream.dart:531: * Returns a new stream that emits the error events and done event I've tried doing a rewording, but I'm not completely satisfied either. It gets complicated by having both "the returned stream" and "the stream returned by [convert]". Too many returned streams. https://codereview.chromium.org/2899273003/diff/1/sdk/lib/async/stream.dart#n... sdk/lib/async/stream.dart:631: * Transform each element of this stream into a sqeuence of elements. On 2017/05/24 13:37:50, floitsch wrote: > Transforms ... sequence ... Done. https://codereview.chromium.org/2899273003/diff/1/sdk/lib/async/stream.dart#n... sdk/lib/async/stream.dart:633: * Returns a new stream which emits the same error and done events as Reworded. It's simpler than the async version. https://codereview.chromium.org/2899273003/diff/1/sdk/lib/async/stream.dart#n... sdk/lib/async/stream.dart:651: * Pipe the events of this stream into [streamConsumer]. On 2017/05/24 13:37:50, floitsch wrote: > Pipes Done. https://codereview.chromium.org/2899273003/diff/1/sdk/lib/async/stream.dart#n... sdk/lib/async/stream.dart:663: * If the call to [StreamConsumer.addStream] fails in some way, this Actually, I meant the completely vague thing here, because addStream is not a function I'm in control of. If it decides to surface an uncaught error, so will this call. If it puts an error into the future it returns, so will this call. Throwing is only one failure mode for an async operation. https://codereview.chromium.org/2899273003/diff/1/sdk/lib/async/stream.dart#n... sdk/lib/async/stream.dart:682: * The [streamTransformer] can return any stream, On 2017/05/24 13:37:50, floitsch wrote: > any stream. > Whether the ... > > Maybe mention, that transform can be used as a building block for all > transforming methods, and that it is usually preferable to use existing methods > if they perform the intended task. Done. https://codereview.chromium.org/2899273003/diff/1/sdk/lib/async/stream.dart#n... sdk/lib/async/stream.dart:792: * and procssing stops. On 2017/05/24 13:37:51, floitsch wrote: > processing Done. https://codereview.chromium.org/2899273003/diff/1/sdk/lib/async/stream.dart#n... sdk/lib/async/stream.dart:1586: * Set or override the data event handler of this subscription. Just "replaces", it's cleaner! https://codereview.chromium.org/2899273003/diff/1/sdk/lib/async/stream.dart#n... sdk/lib/async/stream.dart:1590: * If [handleData] is `null`, furhter elements are ignored. On 2017/05/24 13:37:51, floitsch wrote: > further Done. https://codereview.chromium.org/2899273003/diff/1/sdk/lib/async/stream.dart#n... sdk/lib/async/stream.dart:1661: * When all previously calls to [pause] has been matched by a calls to On 2017/05/24 13:37:51, floitsch wrote: > have been matched Done. https://codereview.chromium.org/2899273003/diff/1/sdk/lib/async/stream.dart#n... sdk/lib/async/stream.dart:1663: * the stream subscription will again emit events. There is no guarantee that it will emit events immediately. To be completely pedantic, it *may* emit events again - but maybe it won't. https://codereview.chromium.org/2899273003/diff/1/sdk/lib/async/stream.dart#n... sdk/lib/async/stream.dart:1668: * Returns true if the [StreamSubscription] is currently paused. On 2017/05/24 13:37:51, floitsch wrote: > Whether the ... Done. https://codereview.chromium.org/2899273003/diff/1/sdk/lib/async/stream.dart#n... sdk/lib/async/stream.dart:2072: /// The stream created by `groupBy` emits a `GroupedEvents` for each distinct key Ack, happened with the renaming :(
Description was changed from ========== Update stream documentation. Fixes #29694 BUG= http://dartbug.com/29694 ========== to ========== Update stream documentation. Fixes #29694 BUG= http://dartbug.com/29694 R=floitsch@google.com Committed: https://github.com/dart-lang/sdk/commit/fbe98e6bfc57fa584e5fcd04b61d1c65a8af0c88 ==========
Message was sent while issue was closed.
Committed patchset #2 (id:20001) manually as fbe98e6bfc57fa584e5fcd04b61d1c65a8af0c88 (presubmit successful).
Message was sent while issue was closed.
https://codereview.chromium.org/2899273003/diff/20001/sdk/lib/async/stream.dart File sdk/lib/async/stream.dart (right): https://codereview.chromium.org/2899273003/diff/20001/sdk/lib/async/stream.da... sdk/lib/async/stream.dart:633: * The event values are proveded as an [Iterable] by a call to [convert] provided https://codereview.chromium.org/2899273003/diff/20001/sdk/lib/async/stream.da... sdk/lib/async/stream.dart:636: * If calling [convert] throws, or if iteration of the returned values throws, if the iteration |
