Add a ScheduledStream class and some stream matchers.

Add a ScheduledStream class and some stream matchers.

R=rnystrom@google.com

Committed: https://code.google.com/p/dart/source/detail?r=31629

[Bob Nystrom, 2013-12-21 00:11:34]

Looks pretty swell!

https://codereview.chromium.org/119673002/diff/1/pkg/scheduled_test/lib/sched...
File pkg/scheduled_test/lib/scheduled_stream.dart (right):

https://codereview.chromium.org/119673002/diff/1/pkg/scheduled_test/lib/sched...
pkg/scheduled_test/lib/scheduled_stream.dart:46: final _pendingValues = new
Queue<Either<T, Pair<dynamic, StackTrace>>>();
Instead of Either<Pair>, how about a concrete type that represents a value or a
failure? That's the only thing you're using Either to represent isn't it?

https://codereview.chromium.org/119673002/diff/1/pkg/scheduled_test/lib/sched...
pkg/scheduled_test/lib/scheduled_stream.dart:48: /// All values emitted by this
stream so far.
This is confusing. "Emitted" implies a push model, but this is really about the
pull model. How about, "All values consumed by this scheduled stream so far."

Likewise the second sentence is confusing.

https://codereview.chromium.org/119673002/diff/1/pkg/scheduled_test/lib/sched...
pkg/scheduled_test/lib/scheduled_stream.dart:58: List<T> get allValues {
The distinction in names here is a bit ambiguous. How about "streamedValues" or
"emittedValues"?

https://codereview.chromium.org/119673002/diff/1/pkg/scheduled_test/lib/sched...
pkg/scheduled_test/lib/scheduled_stream.dart:69: bool _nextPending = false;
How about _isNextPending? "_nextPending" sound to me like "the thing that is
next pending".

Do you need to store this at all? Is it equivalent to _nextCompleter != null? If
not, why not?

https://codereview.chromium.org/119673002/diff/1/pkg/scheduled_test/lib/sched...
pkg/scheduled_test/lib/scheduled_stream.dart:162: .whenComplete(() {
It took me a long time to parse this and realize that .whenComplete is applied
to the result of .match(), not Chain.track(). How about doing:

var future = _pendingValues.removeFirst().match(...)
return future.whenComplete(...);

Also, the future will already be complete in either case, won't it? Is there a
reason you're using whenComplete() here instead of just leaving _nextPending
false?

https://codereview.chromium.org/119673002/diff/1/pkg/scheduled_test/lib/sched...
pkg/scheduled_test/lib/scheduled_stream.dart:227: }
Just to be nice to the GC, may as well clear _forks too.

https://codereview.chromium.org/119673002/diff/1/pkg/scheduled_test/lib/sched...
pkg/scheduled_test/lib/scheduled_stream.dart:245: }
Probably want to clear _nextPending here too.

https://codereview.chromium.org/119673002/diff/1/pkg/scheduled_test/lib/src/s...
File pkg/scheduled_test/lib/src/stream_matcher.dart (right):

https://codereview.chromium.org/119673002/diff/1/pkg/scheduled_test/lib/src/s...
pkg/scheduled_test/lib/src/stream_matcher.dart:62: /// This will fail if the
stream never emits [matcher]
"[matcher]" -> "a value that matches [matcher]."

https://codereview.chromium.org/119673002/diff/1/pkg/scheduled_test/lib/src/s...
pkg/scheduled_test/lib/src/stream_matcher.dart:65: StreamMatcher
consumeThrough(matcher) => new _ConsumeThroughMatcher(matcher);
How about "consumeUntil"?

https://codereview.chromium.org/119673002/diff/1/pkg/scheduled_test/lib/src/s...
pkg/scheduled_test/lib/src/stream_matcher.dart:74: StreamMatcher
either(streamMatcher1, streamMatcher2) =>
My intuition is that "either" would not match both. How about "anyOf" or "any"
and allow more than two?

https://codereview.chromium.org/119673002/diff/1/pkg/scheduled_test/lib/src/s...
pkg/scheduled_test/lib/src/stream_matcher.dart:83: StreamMatcher
allow(streamMatcher) => new _AllowMatcher(streamMatcher);
"skipWhile"?

https://codereview.chromium.org/119673002/diff/1/pkg/scheduled_test/lib/src/s...
pkg/scheduled_test/lib/src/stream_matcher.dart:98: return new
StringDescription("unexpected end of stream");
Describe the matcher here too.

https://codereview.chromium.org/119673002/diff/1/pkg/scheduled_test/lib/src/s...
pkg/scheduled_test/lib/src/stream_matcher.dart:114: final int _n;
Doc.

https://codereview.chromium.org/119673002/diff/1/pkg/scheduled_test/lib/src/s...
pkg/scheduled_test/lib/src/stream_matcher.dart:126: if (!hasNext) return new
StringDescription('unexpected end of stream');
Describe the expected values here.

https://codereview.chromium.org/119673002/diff/1/pkg/scheduled_test/lib/src/s...
pkg/scheduled_test/lib/src/stream_matcher.dart:192: if (!hasNext) return new
StringDescription("unexpected end of stream");
Ditto.

https://codereview.chromium.org/119673002/diff/1/pkg/scheduled_test/lib/src/s...
pkg/scheduled_test/lib/src/stream_matcher.dart:228: stream2.close();
Should these be closed only when *both* futures have completed, or does it make
more sense to add a whenComplete() after each tryMatch()?

https://codereview.chromium.org/119673002/diff/1/pkg/scheduled_test/lib/src/u...
File pkg/scheduled_test/lib/src/utils.dart (right):

https://codereview.chromium.org/119673002/diff/1/pkg/scheduled_test/lib/src/u...
pkg/scheduled_test/lib/src/utils.dart:41: /// It's an error to access this is
this is of type `F`.
"this is this" -> "this if the value"

https://codereview.chromium.org/119673002/diff/1/pkg/scheduled_test/lib/src/u...
pkg/scheduled_test/lib/src/utils.dart:49: /// It's an error to access this is
this is of type `E`.
Ditto.

https://codereview.chromium.org/119673002/diff/1/pkg/scheduled_test/lib/src/u...
pkg/scheduled_test/lib/src/utils.dart:56: Either.withFirst(this._value)
Maybe just Either.first()?

https://codereview.chromium.org/119673002/diff/1/pkg/scheduled_test/test/sche...
File pkg/scheduled_test/test/scheduled_stream/scheduled_stream_test.dart
(right):

https://codereview.chromium.org/119673002/diff/1/pkg/scheduled_test/test/sche...
pkg/scheduled_test/test/scheduled_stream/scheduled_stream_test.dart:395: });
These tests are the same as the previous group, right? Let's refactor that and
make that explicit. I like it when tests show that two things should exhibit the
same behavior by reusing the test itself.

https://codereview.chromium.org/119673002/diff/1/pkg/scheduled_test/test/sche...
File pkg/scheduled_test/test/scheduled_stream/stream_matcher_test.dart (right):

https://codereview.chromium.org/119673002/diff/1/pkg/scheduled_test/test/sche...
pkg/scheduled_test/test/scheduled_stream/stream_matcher_test.dart:48: //
expectTestFails("expect() with a non-matching value fails", () {
Document why these are commented out.

[nweiz, 2014-01-07 03:16:39]

https://codereview.chromium.org/119673002/diff/1/pkg/scheduled_test/lib/sched...
File pkg/scheduled_test/lib/scheduled_stream.dart (right):

https://codereview.chromium.org/119673002/diff/1/pkg/scheduled_test/lib/sched...
pkg/scheduled_test/lib/scheduled_stream.dart:46: final _pendingValues = new
Queue<Either<T, Pair<dynamic, StackTrace>>>();
On 2013/12/21 00:11:34, Bob Nystrom wrote:
> Instead of Either<Pair>, how about a concrete type that represents a value or
a
> failure? That's the only thing you're using Either to represent isn't it?

Good idea. Done.

https://codereview.chromium.org/119673002/diff/1/pkg/scheduled_test/lib/sched...
pkg/scheduled_test/lib/scheduled_stream.dart:48: /// All values emitted by this
stream so far.
On 2013/12/21 00:11:34, Bob Nystrom wrote:
> This is confusing. "Emitted" implies a push model, but this is really about
the
> pull model. How about, "All values consumed by this scheduled stream so far."
> 
> Likewise the second sentence is confusing.

"Consumed" is also weird because it sounds like it's referring to the values
consumed from the underlying stream.

I don't love "emitted", but I also don't mind it, since I think it makes a fair
amount of sense to say that a user requests a value that later gets emitted.

https://codereview.chromium.org/119673002/diff/1/pkg/scheduled_test/lib/sched...
pkg/scheduled_test/lib/scheduled_stream.dart:58: List<T> get allValues {
On 2013/12/21 00:11:34, Bob Nystrom wrote:
> The distinction in names here is a bit ambiguous. How about "streamedValues"
or
> "emittedValues"?

Done.

https://codereview.chromium.org/119673002/diff/1/pkg/scheduled_test/lib/sched...
pkg/scheduled_test/lib/scheduled_stream.dart:69: bool _nextPending = false;
On 2013/12/21 00:11:34, Bob Nystrom wrote:
> How about _isNextPending? "_nextPending" sound to me like "the thing that is
> next pending".

Done.

> Do you need to store this at all? Is it equivalent to _nextCompleter != null?
If
> not, why not?

Added a comment about this.

https://codereview.chromium.org/119673002/diff/1/pkg/scheduled_test/lib/sched...
pkg/scheduled_test/lib/scheduled_stream.dart:162: .whenComplete(() {
On 2013/12/21 00:11:34, Bob Nystrom wrote:
> It took me a long time to parse this and realize that .whenComplete is applied
> to the result of .match(), not Chain.track(). How about doing:
> 
> var future = _pendingValues.removeFirst().match(...)
> return future.whenComplete(...);

I changed this as part of replacing Either with FallibleValue.

> Also, the future will already be complete in either case, won't it? Is there a
> reason you're using whenComplete() here instead of just leaving _nextPending
> false?

There's a period of time between when [Future.value] and [Future.error] are
initialized and when they fire. Other events can happen during this time.

https://codereview.chromium.org/119673002/diff/1/pkg/scheduled_test/lib/sched...
pkg/scheduled_test/lib/scheduled_stream.dart:227: }
On 2013/12/21 00:11:34, Bob Nystrom wrote:
> Just to be nice to the GC, may as well clear _forks too.

Done.

https://codereview.chromium.org/119673002/diff/1/pkg/scheduled_test/lib/sched...
pkg/scheduled_test/lib/scheduled_stream.dart:245: }
On 2013/12/21 00:11:34, Bob Nystrom wrote:
> Probably want to clear _nextPending here too.

Done.

https://codereview.chromium.org/119673002/diff/1/pkg/scheduled_test/lib/src/s...
File pkg/scheduled_test/lib/src/stream_matcher.dart (right):

https://codereview.chromium.org/119673002/diff/1/pkg/scheduled_test/lib/src/s...
pkg/scheduled_test/lib/src/stream_matcher.dart:62: /// This will fail if the
stream never emits [matcher]
On 2013/12/21 00:11:34, Bob Nystrom wrote:
> "[matcher]" -> "a value that matches [matcher]."

Done.

https://codereview.chromium.org/119673002/diff/1/pkg/scheduled_test/lib/src/s...
pkg/scheduled_test/lib/src/stream_matcher.dart:65: StreamMatcher
consumeThrough(matcher) => new _ConsumeThroughMatcher(matcher);
On 2013/12/21 00:11:34, Bob Nystrom wrote:
> How about "consumeUntil"?

That's what this was originally, but I wanted it to be clear that it consumed
the value that matched [matcher] as well.

https://codereview.chromium.org/119673002/diff/1/pkg/scheduled_test/lib/src/s...
pkg/scheduled_test/lib/src/stream_matcher.dart:74: StreamMatcher
either(streamMatcher1, streamMatcher2) =>
On 2013/12/21 00:11:34, Bob Nystrom wrote:
> My intuition is that "either" would not match both. How about "anyOf" or "any"
> and allow more than two?

I think it makes sense for "either" to match both branches. I'd expect something
matching one and only one to be called "xor" or "eitherButNotBoth". I could
change this to "eitherOrBoth" if you prefer.

I think extending this to match any number of matchers would make the code
considerably more complicated, and worse would make the string descriptions more
difficult to read. I also don't think it would provide much benefit.

https://codereview.chromium.org/119673002/diff/1/pkg/scheduled_test/lib/src/s...
pkg/scheduled_test/lib/src/stream_matcher.dart:83: StreamMatcher
allow(streamMatcher) => new _AllowMatcher(streamMatcher);
On 2013/12/21 00:11:34, Bob Nystrom wrote:
> "skipWhile"?

That implies that it will skip more than one value, which is incorrect.

https://codereview.chromium.org/119673002/diff/1/pkg/scheduled_test/lib/src/s...
pkg/scheduled_test/lib/src/stream_matcher.dart:98: return new
StringDescription("unexpected end of stream");
On 2013/12/21 00:11:34, Bob Nystrom wrote:
> Describe the matcher here too.

This description will be provided elsewhere in the expectation message.

https://codereview.chromium.org/119673002/diff/1/pkg/scheduled_test/lib/src/s...
pkg/scheduled_test/lib/src/stream_matcher.dart:114: final int _n;
On 2013/12/21 00:11:34, Bob Nystrom wrote:
> Doc.

Usually we don't document private instance variables that come directly from
functions that have their own documentation. In this case, [_n] is the same as
[n] in [nextValues].

https://codereview.chromium.org/119673002/diff/1/pkg/scheduled_test/lib/src/s...
pkg/scheduled_test/lib/src/stream_matcher.dart:126: if (!hasNext) return new
StringDescription('unexpected end of stream');
On 2013/12/21 00:11:34, Bob Nystrom wrote:
> Describe the expected values here.

See above.

https://codereview.chromium.org/119673002/diff/1/pkg/scheduled_test/lib/src/s...
pkg/scheduled_test/lib/src/stream_matcher.dart:192: if (!hasNext) return new
StringDescription("unexpected end of stream");
On 2013/12/21 00:11:34, Bob Nystrom wrote:
> Ditto.

See above.

https://codereview.chromium.org/119673002/diff/1/pkg/scheduled_test/lib/src/s...
pkg/scheduled_test/lib/src/stream_matcher.dart:228: stream2.close();
On 2013/12/21 00:11:34, Bob Nystrom wrote:
> Should these be closed only when *both* futures have completed, or does it
make
> more sense to add a whenComplete() after each tryMatch()?

Done.

https://codereview.chromium.org/119673002/diff/1/pkg/scheduled_test/lib/src/u...
File pkg/scheduled_test/lib/src/utils.dart (right):

https://codereview.chromium.org/119673002/diff/1/pkg/scheduled_test/lib/src/u...
pkg/scheduled_test/lib/src/utils.dart:41: /// It's an error to access this is
this is of type `F`.
On 2013/12/21 00:11:34, Bob Nystrom wrote:
> "this is this" -> "this if the value"

N/A

https://codereview.chromium.org/119673002/diff/1/pkg/scheduled_test/lib/src/u...
pkg/scheduled_test/lib/src/utils.dart:49: /// It's an error to access this is
this is of type `E`.
On 2013/12/21 00:11:34, Bob Nystrom wrote:
> Ditto.

N/A

https://codereview.chromium.org/119673002/diff/1/pkg/scheduled_test/lib/src/u...
pkg/scheduled_test/lib/src/utils.dart:56: Either.withFirst(this._value)
On 2013/12/21 00:11:34, Bob Nystrom wrote:
> Maybe just Either.first()?

N/A

https://codereview.chromium.org/119673002/diff/1/pkg/scheduled_test/test/sche...
File pkg/scheduled_test/test/scheduled_stream/scheduled_stream_test.dart
(right):

https://codereview.chromium.org/119673002/diff/1/pkg/scheduled_test/test/sche...
pkg/scheduled_test/test/scheduled_stream/scheduled_stream_test.dart:395: });
On 2013/12/21 00:11:34, Bob Nystrom wrote:
> These tests are the same as the previous group, right? Let's refactor that and
> make that explicit. I like it when tests show that two things should exhibit
the
> same behavior by reusing the test itself.

There are some shared tests, but others (e.g. the ones to do with [values] and
[allValues]) that aren't. I could try to factor out a function of some sort, but
I think the mechanics of passing in the stream to each test would make that add
more complexity than it would remove.

https://codereview.chromium.org/119673002/diff/1/pkg/scheduled_test/test/sche...
File pkg/scheduled_test/test/scheduled_stream/stream_matcher_test.dart (right):

https://codereview.chromium.org/119673002/diff/1/pkg/scheduled_test/test/sche...
pkg/scheduled_test/test/scheduled_stream/stream_matcher_test.dart:48: //
expectTestFails("expect() with a non-matching value fails", () {
On 2013/12/21 00:11:34, Bob Nystrom wrote:
> Document why these are commented out.

It looks like issue 15740 is no longer causing problems here, so I'm
uncommenting them for now.

[Bob Nystrom, 2014-01-08 00:47:40]

A couple of suggestions but LGTM either way!

https://codereview.chromium.org/119673002/diff/60001/pkg/barback/lib/src/util...
File pkg/barback/lib/src/utils.dart (right):

https://codereview.chromium.org/119673002/diff/60001/pkg/barback/lib/src/util...
pkg/barback/lib/src/utils.dart:13: class FallibleValue<E> {
What do you think of just calling this "Fallible"? That would read nicely with
the type argument as the noun. Ex: Fallible<int>

https://codereview.chromium.org/119673002/diff/60001/pkg/barback/lib/src/util...
pkg/barback/lib/src/utils.dart:17: /// Whether [this] has an [error], as opposed
to an[value].
"an[" -> "a ["

https://codereview.chromium.org/119673002/diff/60001/pkg/barback/lib/src/util...
pkg/barback/lib/src/utils.dart:61: FallibleValue.withValue(this._value)
Maybe just:

new Fallible.value(...)
new Fallible.error(...)

https://codereview.chromium.org/119673002/diff/60001/pkg/scheduled_test/lib/s...
File pkg/scheduled_test/lib/scheduled_stream.dart (right):

https://codereview.chromium.org/119673002/diff/60001/pkg/scheduled_test/lib/s...
pkg/scheduled_test/lib/scheduled_stream.dart:192: return new
Future.error(valueOrError.error, valueOrError.stackTrace);
Given how nicely it maps 1-1, you could just add a .toFuture() to FallibleValue.

[nweiz, 2014-01-08 22:14:39]

Committed patchset #3 manually as r31629 (presubmit successful).

[nweiz, 2014-01-08 22:15:09]

https://codereview.chromium.org/119673002/diff/60001/pkg/barback/lib/src/util...
File pkg/barback/lib/src/utils.dart (right):

https://codereview.chromium.org/119673002/diff/60001/pkg/barback/lib/src/util...
pkg/barback/lib/src/utils.dart:13: class FallibleValue<E> {
On 2014/01/08 00:47:40, Bob Nystrom wrote:
> What do you think of just calling this "Fallible"? That would read nicely with
> the type argument as the noun. Ex: Fallible<int>

Done.

https://codereview.chromium.org/119673002/diff/60001/pkg/barback/lib/src/util...
pkg/barback/lib/src/utils.dart:17: /// Whether [this] has an [error], as opposed
to an[value].
On 2014/01/08 00:47:40, Bob Nystrom wrote:
> "an[" -> "a ["

Done.

https://codereview.chromium.org/119673002/diff/60001/pkg/barback/lib/src/util...
pkg/barback/lib/src/utils.dart:61: FallibleValue.withValue(this._value)
On 2014/01/08 00:47:40, Bob Nystrom wrote:
> Maybe just:
> 
> new Fallible.value(...)
> new Fallible.error(...)

That's what I had at first, but those apparently conflict with the [value] and
[error] getters.

https://codereview.chromium.org/119673002/diff/60001/pkg/scheduled_test/lib/s...
File pkg/scheduled_test/lib/scheduled_stream.dart (right):

https://codereview.chromium.org/119673002/diff/60001/pkg/scheduled_test/lib/s...
pkg/scheduled_test/lib/scheduled_stream.dart:192: return new
Future.error(valueOrError.error, valueOrError.stackTrace);
On 2014/01/08 00:47:40, Bob Nystrom wrote:
> Given how nicely it maps 1-1, you could just add a .toFuture() to
FallibleValue.

Done.