-
Notifications
You must be signed in to change notification settings - Fork 21
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add whereType transformer #60
Changes from 3 commits
ee545a8
9449f17
d003e1f
95aa506
a319ebc
7b52759
59ce371
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,3 +1,7 @@ | ||
## 0.0.15 | ||
|
||
- Add `whereType`. | ||
|
||
## 0.0.14+1 | ||
|
||
- Allow using non-dev Dart 2 SDK. | ||
|
Original file line number | Diff line number | Diff line change | ||
---|---|---|---|---|
@@ -0,0 +1,52 @@ | ||||
// Copyright (c) 2018, the Dart project authors. Please see the AUTHORS file | ||||
// for details. All rights reserved. Use of this source code is governed by a | ||||
// BSD-style license that can be found in the LICENSE file. | ||||
|
||||
import 'dart:async'; | ||||
|
||||
/// Emits only the events which have type [R]. | ||||
/// | ||||
/// If the source stream is a broadcast stream the result will be as well. | ||||
/// | ||||
/// Errors from the source stream are forwarded directly to the result stream. | ||||
/// | ||||
/// The static type of the returned transformer takes `Null` so that it can | ||||
/// satisfy the subtype requirements for `stream.transform()` argument on any | ||||
/// source Stream. The argument to `bind` has been broaded to take | ||||
/// `Stream<Object>` since it never be passed a `Stream<Null>` at runtime. This | ||||
/// is safe to use on any source stream and there is no static or runtime | ||||
/// checking that [R] is sensible - that is that is a subtype of the stream's | ||||
/// type such that some values of that type may be possible. | ||||
StreamTransformer<Null, R> whereType<R>() => _WhereType<R>(); | ||||
|
||||
class _WhereType<R> extends StreamTransformerBase<Null, R> { | ||||
@override | ||||
Stream<R> bind(Stream<Object> values) { | ||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. nit: maybe There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This matches the naming pattern used elsewhere in the package.
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Do you think its actually a good name though? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think it's about as good as anything else. A There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
In the case of a stream transformer I would disagree - it is transforming one stream into another stream, so it makes sense to me to consider one the "inputs" stream and one the "outputs" stream. If I see the word "values" in the middle of the method its not clear what that represents. Is that the outputs or the inputs? But if I see "inputs" I can immediately assume its the input stream. In any case I don't care enough to withhold an lgtm, but I do think "inputs" is better :).
Right, because listen doesn't transform anything, it only listens for values and there is only one stream in scope. With a transformer there are two streams in the scope of the |
||||
var controller = values.isBroadcast | ||||
? StreamController<R>.broadcast(sync: true) | ||||
: StreamController<R>(sync: true); | ||||
|
||||
StreamSubscription<Object> subscription; | ||||
controller.onListen = () { | ||||
if (subscription != null) return; | ||||
subscription = values.listen( | ||||
(value) { | ||||
if (value is R) controller.add(value); | ||||
}, | ||||
onError: controller.addError, | ||||
onDone: () { | ||||
subscription = null; | ||||
controller.close(); | ||||
}); | ||||
if (!values.isBroadcast) { | ||||
controller.onPause = subscription.pause; | ||||
controller.onResume = subscription.resume; | ||||
} | ||||
controller.onCancel = () { | ||||
subscription?.cancel(); | ||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. this used to return this value, not sure if it matters or not There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Hmm, I'm not sure either. There was some weirdness with an analyzer hint around this at one point that could have been why I had it in the |
||||
subscription = null; | ||||
}; | ||||
}; | ||||
return controller.stream; | ||||
} | ||||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -2,7 +2,7 @@ name: stream_transform | |
description: A collection of utilities to transform and manipulate streams. | ||
author: Dart Team <[email protected]> | ||
homepage: https://www.github.com/dart-lang/stream_transform | ||
version: 0.0.15-dev | ||
version: 0.0.15 | ||
|
||
environment: | ||
sdk: ">=2.1.0 <3.0.0" | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,50 @@ | ||
// Copyright (c) 2018, the Dart project authors. Please see the AUTHORS file | ||
// for details. All rights reserved. Use of this source code is governed by a | ||
// BSD-style license that can be found in the LICENSE file. | ||
|
||
import 'dart:async'; | ||
|
||
import 'package:test/test.dart'; | ||
|
||
import 'package:stream_transform/stream_transform.dart'; | ||
|
||
void main() { | ||
test('forwards only events that match the type', () async { | ||
var values = Stream.fromIterable([1, 'a', 2, 'b']); | ||
var filtered = values.transform(whereType<String>()); | ||
expect(await filtered.toList(), ['a', 'b']); | ||
}); | ||
|
||
test('can result in empty stream', () async { | ||
var values = Stream.fromIterable([1, 2, 3, 4]); | ||
var filtered = values.transform(whereType<String>()); | ||
expect(await filtered.isEmpty, true); | ||
}); | ||
|
||
test('forwards values to multiple listeners', () async { | ||
var values = StreamController.broadcast(); | ||
var filtered = values.stream.transform(whereType<String>()); | ||
var firstValues = []; | ||
var secondValues = []; | ||
filtered..listen(firstValues.add)..listen(secondValues.add); | ||
values..add(1)..add('a')..add(2)..add('b'); | ||
await Future(() {}); | ||
expect(firstValues, ['a', 'b']); | ||
expect(secondValues, ['a', 'b']); | ||
}); | ||
|
||
test('closes streams with multiple listeners', () async { | ||
var values = StreamController.broadcast(); | ||
var filtered = values.stream.transform(whereType<String>()); | ||
var firstDone = false; | ||
var secondDone = false; | ||
filtered | ||
..listen(null, onDone: () => firstDone = true) | ||
..listen(null, onDone: () => secondDone = true); | ||
values.add(1); | ||
values.add('a'); | ||
await values.close(); | ||
expect(firstDone, true); | ||
expect(secondDone, true); | ||
}); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The null is a code smell here, what does it represent? can it also be generic, or should it maybe be Object?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh I see you commented about this, I don't fully understand the issue here but I can check out the repo and try it out which would probably make it more obvious.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
See some discussion here:
dart-lang/language#180 (comment)
concrete scenario:
In this case the
transform
excepts an argument assignable toStreamTransformer<num, S>
. Since thewhereType
call doesn't have a way to statically know thenum
there, it either needs us to be explicit about it (numbers.transform(whereType<num, int>()
), or to use some default that can fill in for anyT
that may have been on the stream.Null
is the only possibility there for now, I'm not sure if we've figured out what the user referenceable bottom type will be after NNBD...There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would at least leave a lengthy doc comment here, its going to be pretty bizarre for anybody reading this in the future so some context would certainly help :).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added a note to the doc comment, how does that look to you? I think for further details the
git blame
should point here which will have extra detail.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think what you added is fine