Add whereType transformer #60

natebosch · 2019-02-11T18:11:53Z

This needs to be implemented as it's own class instead of using
one of the helpers like fromHandlers because it has unique generic
type requirements. We want to avoid requring the call site to re-specify
the generic type of the input stream. The return type needs to be a
StreamTransformer<Null, R> to allow to satisfy static checking.
However the argument to bind at runtime will never be a Stream<Null>
so we need to define it explicitly to widen the allowed argument.

This needs to be implemented as it's own class instead of using one of the helpers like `fromHandlers` because it has unique generic type requirements. We want to avoid requring the call site to re-specify the generic type of the input stream. The return type needs to be a `StreamTransformer<Null, R>` to allow to satisfy static checking. However the argument to `bind` at runtime will never be a `Stream<Null>` so we need to define it explicitly to widen the allowed argument.

jakemac53 · 2019-02-12T17:48:18Z

lib/src/where_type.dart

+/// Errors from the source stream are forwarded directly to the result stream.
+StreamTransformer<Null, R> whereType<R>() => _WhereType<R>();
+
+class _WhereType<R> extends StreamTransformerBase<Null, R> {


The null is a code smell here, what does it represent? can it also be generic, or should it maybe be Object?

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.

See some discussion here:
dart-lang/language#180 (comment)

concrete scenario:

var numbers = Stream<num>.fromIterable([1, 1.5, 2]); var ints = numbers.transform(whereType<int>());

In this case the transform excepts an argument assignable to StreamTransformer<num, S>. Since the whereType call doesn't have a way to statically know the num 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 any T 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...

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 :).

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.

I think what you added is fine

jakemac53 · 2019-02-12T17:50:59Z

lib/src/where_type.dart

+
+class _WhereType<R> extends StreamTransformerBase<Null, R> {
+  @override
+  Stream<R> bind(Stream<Object> values) {


nit: maybe inputs instead of values? values is fairly ambiguous

This matches the naming pattern used elsewhere in the package.

stream_transform/lib/src/from_handlers.dart

Line 50 in 5c50493

Stream<T> bind(Stream<S> values) {

Do you think its actually a good name though?

I think it's about as good as anything else. A value or event is what is emitted by a stream when we don't know anything more specific about it, and I generally like to name a stream as the plural of whatever flows through it. I don't like the name inputs because I wouldn't call the thing emitted by the stream an input - I wouldn't name the argument to the listen callback that.

I don't like the name inputs because I wouldn't call the thing emitted by the stream an input

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 :).

I wouldn't name the argument to the listen callback that.

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 bind method.

jakemac53 · 2019-02-12T17:51:50Z

lib/src/where_type.dart

+      controller.onCancel = () {
+        var toCancel = subscription;
+        subscription = null;
+        if (!valuesDone) return toCancel.cancel();


It seems like it would be cleaner to null out the subscription field when the values are done, and get rid of this valuesDone field entirely.

I think I was worried about the onListen getting called again in a broadcast stream after the values where already done and the subscription != null condition not blocking us from doing work we shouldn't - but it looks like maybe StreamController handles this for us and won't call onListen again anyway. Done

jakemac53 · 2019-02-12T18:04:37Z

As a more general comment, stream does already have a where method... a stream transformer seems quite heavy for this versus just doing stream.where((thing) => thing is SomeType)?

natebosch · 2019-02-12T18:27:59Z

a stream transformer seems quite heavy for this versus just doing stream.where((thing) => thing is SomeType)?

That doesn't end up with the correct static or runtime type. You either need to chain a .cast() call, or use a hack like stream.expand<SomeType>((thing) => thing is SomeType ? [thing] : []). Both of these incur some amount of additional runtime overhead, either to construct a list or to do the cast that will be guaranteed to succeed.

jakemac53 · 2019-02-12T19:22:55Z

lib/src/where_type.dart

+/// Errors from the source stream are forwarded directly to the result stream.
+StreamTransformer<Null, R> whereType<R>() => _WhereType<R>();
+
+class _WhereType<R> extends StreamTransformerBase<Null, R> {


I think what you added is fine

jakemac53 · 2019-02-12T19:42:38Z

lib/src/where_type.dart

+
+class _WhereType<R> extends StreamTransformerBase<Null, R> {
+  @override
+  Stream<R> bind(Stream<Object> values) {


I don't like the name inputs because I wouldn't call the thing emitted by the stream an input

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 :).

I wouldn't name the argument to the listen callback that.

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 bind method.

jakemac53 · 2019-02-12T19:45:50Z

lib/src/where_type.dart

            controller.close();
          });
      if (!values.isBroadcast) {
        controller.onPause = subscription.pause;
        controller.onResume = subscription.resume;
      }
      controller.onCancel = () {
-        var toCancel = subscription;
+        subscription?.cancel();


this used to return this value, not sure if it matters or not

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 fromHandlers implementation, and I copied most of that here.

natebosch requested review from lrhn and jakemac53 February 11, 2019 18:11

jakemac53 reviewed Feb 12, 2019

View reviewed changes

natebosch added 2 commits February 12, 2019 10:55

Use null state of subscription instead of valuesDone

9449f17

Include note on typing in doc comment

d003e1f

jakemac53 approved these changes Feb 12, 2019

View reviewed changes

natebosch added 4 commits February 12, 2019 12:02

Rephrase commend and rename to input

95aa506

rename to source

a319ebc

spelling

7b52759

More typos

59ce371

natebosch merged commit 1029f4c into master Feb 12, 2019

natebosch deleted the where-type branch February 12, 2019 23:43

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add whereType transformer #60

Add whereType transformer #60

natebosch commented Feb 11, 2019

jakemac53 Feb 12, 2019

jakemac53 Feb 12, 2019

natebosch Feb 12, 2019

jakemac53 Feb 12, 2019 •

edited

Loading

natebosch Feb 12, 2019

jakemac53 Feb 12, 2019

jakemac53 Feb 12, 2019

natebosch Feb 12, 2019

jakemac53 Feb 12, 2019

natebosch Feb 12, 2019

jakemac53 Feb 12, 2019

jakemac53 Feb 12, 2019

natebosch Feb 12, 2019

jakemac53 commented Feb 12, 2019 •

edited

Loading

natebosch commented Feb 12, 2019

jakemac53 Feb 12, 2019

jakemac53 Feb 12, 2019

jakemac53 Feb 12, 2019

natebosch Feb 12, 2019 •

edited

Loading

Add whereType transformer #60

Add whereType transformer #60

Conversation

natebosch commented Feb 11, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jakemac53 Feb 12, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jakemac53 commented Feb 12, 2019 • edited Loading

natebosch commented Feb 12, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

natebosch Feb 12, 2019 • edited Loading

Choose a reason for hiding this comment

jakemac53 Feb 12, 2019 •

edited

Loading

jakemac53 commented Feb 12, 2019 •

edited

Loading

natebosch Feb 12, 2019 •

edited

Loading