Add a StreamSink.rejectErrors() extension method #169
Conversation
This makes it easy for authors to expose sinks that can't natively consume errors, but still handle them in a consistent and robust manner.
lib/src/stream_sink_extensions.dart
Outdated
| transformer.bind(this); | ||
|
|
||
| /// Returns a [StreamSink] that forwards to [this], but doesn't allow any | ||
| /// errors to be passed in. |
There was a problem hiding this comment.
First line should be shorter.
The [this] is not a valid doc link (not an identifier, not a source declaration).
Generally, never start a dartdoc with "Returns".
Maybe:
/// Transforms this stream sink into one which rejects errors.
///
/// Returns a new [StreamSink]. Any non-error event added to that new sink,
/// whether by [add], [close] or [addStream], is also added to this stream sink.
/// Any error event added to the new sink, whether by [addError] or [addStream],
/// will instead close this sinks, the error becomes the result of the returned
/// sink's [StreamSink.done] future, and further events added to the new sink
/// are ignored.
There was a problem hiding this comment.
First line should be shorter.
Done.
The
[this]is not a valid doc link (not an identifier, not a source declaration).
I don't think this is accurate—the SDK has over 100 instances of [this], and it does seem to be linked by dartdoc (see this example).
Generally, never start a dartdoc with "Returns".
That's contrary to Effective Dart's suggestion.
There was a problem hiding this comment.
TIL that [this] is valid and links to the class itself. I wish DartDoc had, well, documentation (I still don't remember how to link to an unnamed constructor).
I disagree with the Effective Dart example here. The suggestion to use active verbs is fine, it's just that the word "Returns" is always inferior to some other word (or none, for getter-like functions where you should use a noun phrase).
If the only or most important thing you can say about a function is which object it returns, it should be treated as a getter-like function ("parameterized getter") and documented as such (noun phrase).
Most of the time, it's not the most important thing, and the operation performed by the function is more relevant. In this case, "Creates" is a better word since it emphasized that the returned sink is new.
So, it's not that "Returns" is technically wrong, I've just found that it's just never the best word to use. Seeing it means that the text can be improved.
(Having a later section starting with "Returns ..." is highly recommended as the way to describe the returned value. Users should be trained to look for such a section. That too gets muddled when the first overview sentence starts with "Returns" as well).
| var addStreamCompleter = _addStreamCompleter = Completer.sync(); | ||
| _addStreamSubscription = stream.listen(_inner.add, | ||
| onError: _addError, onDone: addStreamCompleter.complete); | ||
| return addStreamCompleter.future.then((_) { |
There was a problem hiding this comment.
If the addStreamCompleter.future completes with an error then that error will be unhandled here. Add an onError: (_) {} to the then call as well.
(I think it's fine to not clear the fields in the error branch, the only way that future can complete with an error from one of the places above where the complete call is followed by clearing the fields too. Might be worth a comment.)
There was a problem hiding this comment.
This future is returned, so the error should be handled by the caller.
There was a problem hiding this comment.
ACK, my bad.
The comment request stands, it's probably true that you don't need to clear the fields on an error, but it's not clear from reading just this code.
My first instinct was to add an onError: (_) { /* clear fields */} argument or change this one to a .whenComplete. That's unnecessary, but not clearly so.
This makes it easy for authors to expose sinks that can't natively
consume errors, but still handle them in a consistent and robust
manner.