Add collectBytesCancelable function.

Add collectBytesCancelable function.

R=nweiz@google.com

Committed: https://github.com/dart-lang/async/commit/f7c1f4f3413d0f56988773261b33b66e316ae45b

[Lasse Reichstein Nielsen, 2017-01-27 12:20:47]

lrn@google.com changed reviewers:
+ nweiz@google.com

[Lasse Reichstein Nielsen, 2017-01-27 12:20:48]

https://codereview.chromium.org/2661603002/diff/1/lib/src/byte_collector.dart
File lib/src/byte_collector.dart (left):

https://codereview.chromium.org/2661603002/diff/1/lib/src/byte_collector.dart...
lib/src/byte_collector.dart:16: var byteLists = List<List<int>>[];
The type here was actually wrong (I just moved it from left to right without
removing the outer List), so this CL also fixes that bug.

[nweiz, 2017-01-27 21:56:01]

https://codereview.chromium.org/2661603002/diff/1/lib/src/byte_collector.dart
File lib/src/byte_collector.dart (right):

https://codereview.chromium.org/2661603002/diff/1/lib/src/byte_collector.dart...
lib/src/byte_collector.dart:17: return _collectBytes<Future<Uint8List>>(source,
_futureResult);
I think you shouldn't need an explicit generic type here (or below)—it should
infer it from the return type of the function, and from the callback as well.

If you do include the generic, either make it comment-style or update the SDK
constraint to have 1.21.0 as its minimum.

https://codereview.chromium.org/2661603002/diff/1/lib/src/byte_collector.dart...
lib/src/byte_collector.dart:38: completer.future;
I'd make these inline—I think it reads clearer, and you get to avoid re-writing
a bunch of type annotations.

https://codereview.chromium.org/2661603002/diff/1/lib/src/byte_collector.dart...
lib/src/byte_collector.dart:56: Completer<Uint8List> completer)) {
I'd just pass the Future here, since that's the only thing that's actually used.

https://codereview.chromium.org/2661603002/diff/1/lib/src/byte_collector.dart...
lib/src/byte_collector.dart:70: return result(subscription, completer);
I really wish we had efficient multiple returns -_-.

https://codereview.chromium.org/2661603002/diff/1/test/byte_collection_test.dart
File test/byte_collection_test.dart (right):

https://codereview.chromium.org/2661603002/diff/1/test/byte_collection_test.d...
test/byte_collection_test.dart:9: import "package:async/async.dart" show
collectBytes, collectBytesCancelable;
I don't think it's worth maintaining a "show" list unless there's likely to be a
concrete collision.

[Lasse Reichstein Nielsen, 2017-01-30 15:06:34]

https://codereview.chromium.org/2661603002/diff/1/lib/src/byte_collector.dart
File lib/src/byte_collector.dart (right):

https://codereview.chromium.org/2661603002/diff/1/lib/src/byte_collector.dart...
lib/src/byte_collector.dart:17: return _collectBytes<Future<Uint8List>>(source,
_futureResult);
It probably doesn't even need to be generic, I can just make it dynamic and do a
down-cast. Not sure what is better in the long run.

Will update to 1.21.0

https://codereview.chromium.org/2661603002/diff/1/lib/src/byte_collector.dart...
lib/src/byte_collector.dart:38: completer.future;
Done.

https://codereview.chromium.org/2661603002/diff/1/lib/src/byte_collector.dart...
lib/src/byte_collector.dart:56: Completer<Uint8List> completer)) {
True. Might as well extract it early instead of late.

https://codereview.chromium.org/2661603002/diff/1/lib/src/byte_collector.dart...
lib/src/byte_collector.dart:70: return result(subscription, completer);
Not sure what "multiple returns" means here, but I'm sure I wish for it in any
case :)

https://codereview.chromium.org/2661603002/diff/1/test/byte_collection_test.dart
File test/byte_collection_test.dart (right):

https://codereview.chromium.org/2661603002/diff/1/test/byte_collection_test.d...
test/byte_collection_test.dart:9: import "package:async/async.dart" show
collectBytes, collectBytesCancelable;
On 2017/01/27 21:56:00, nweiz wrote:
> I don't think it's worth maintaining a "show" list unless there's likely to be
a
> concrete collision.

Done.

[nweiz, 2017-01-30 22:21:26]

It looks like you may have forgotten to upload the most recent change.

https://codereview.chromium.org/2661603002/diff/1/lib/src/byte_collector.dart
File lib/src/byte_collector.dart (right):

https://codereview.chromium.org/2661603002/diff/1/lib/src/byte_collector.dart...
lib/src/byte_collector.dart:17: return _collectBytes<Future<Uint8List>>(source,
_futureResult);
On 2017/01/30 15:06:33, Lasse Reichstein Nielsen wrote:
> It probably doesn't even need to be generic, I can just make it dynamic and do
a
> down-cast. Not sure what is better in the long run.
> 
> Will update to 1.21.0

It does need to be generic in strong mode—but I believe it'll complain
statically if the generic type is missing or doesn't match up.

https://codereview.chromium.org/2661603002/diff/1/lib/src/byte_collector.dart...
lib/src/byte_collector.dart:70: return result(subscription, completer);
On 2017/01/30 15:06:33, Lasse Reichstein Nielsen wrote:
> Not sure what "multiple returns" means here, but I'm sure I wish for it in any
> case :)

So we could write "return subscription, completer;" here and "var subscription,
completer = _collectBytes(source);" in callsites.

[Lasse Reichstein Nielsen, 2017-01-31 11:53:39]

On 2017/01/30 22:21:26, nweiz wrote:
> It looks like you may have forgotten to upload the most recent change.

I also forgot to do the changes — so actually, I mailed the comments too early. 
Changes now done.

[Lasse Reichstein Nielsen, 2017-01-31 11:53:55]

https://codereview.chromium.org/2661603002/diff/1/lib/src/byte_collector.dart
File lib/src/byte_collector.dart (right):

https://codereview.chromium.org/2661603002/diff/1/lib/src/byte_collector.dart...
lib/src/byte_collector.dart:17: return _collectBytes<Future<Uint8List>>(source,
_futureResult);
I've removed all the explicit type declarations (even more now that the
functions are inline), and the analyzer is happy.

https://codereview.chromium.org/2661603002/diff/1/lib/src/byte_collector.dart...
lib/src/byte_collector.dart:70: return result(subscription, completer);
Oh, I absolutely would love to have that.
It's so annoying that you can call a method with two arguments, but not get two
results back.
(As a continuation-passing-style aficionado, I see no difference between the two
:).

[nweiz, 2017-01-31 22:23:10]

https://codereview.chromium.org/2661603002/diff/1/test/byte_collection_test.dart
File test/byte_collection_test.dart (right):

https://codereview.chromium.org/2661603002/diff/1/test/byte_collection_test.d...
test/byte_collection_test.dart:9: import "package:async/async.dart" show
collectBytes, collectBytesCancelable;
On 2017/01/30 15:06:33, Lasse Reichstein Nielsen wrote:
> On 2017/01/27 21:56:00, nweiz wrote:
> > I don't think it's worth maintaining a "show" list unless there's likely to
be
> a
> > concrete collision.
> 
> Done.

I think you missed this one.

https://codereview.chromium.org/2661603002/diff/20001/pubspec.yaml
File pubspec.yaml (right):

https://codereview.chromium.org/2661603002/diff/20001/pubspec.yaml#newcode2
pubspec.yaml:2: version: 1.21.0
I think you meant to change the SDK constraint here...

[Lasse Reichstein Nielsen, 2017-02-01 12:05:43]

https://codereview.chromium.org/2661603002/diff/1/test/byte_collection_test.dart
File test/byte_collection_test.dart (right):

https://codereview.chromium.org/2661603002/diff/1/test/byte_collection_test.d...
test/byte_collection_test.dart:9: import "package:async/async.dart" show
collectBytes, collectBytesCancelable;
ACK. Fixing.

https://codereview.chromium.org/2661603002/diff/20001/pubspec.yaml
File pubspec.yaml (right):

https://codereview.chromium.org/2661603002/diff/20001/pubspec.yaml#newcode2
pubspec.yaml:2: version: 1.21.0
Uhm, DOH!

[nweiz, 2017-02-01 20:42:42]

lgtm!

[Lasse Reichstein Nielsen, 2017-02-03 15:24:10]

Description was changed from

==========
Add collectBytesCancelable function.
==========

to

==========
Add collectBytesCancelable function.

R=nweiz@google.com

Committed:
https://github.com/dart-lang/async/commit/f7c1f4f3413d0f56988773261b33b66e316...
==========

[Lasse Reichstein Nielsen, 2017-02-03 15:24:13]

Committed patchset #4 (id:60001) manually as
f7c1f4f3413d0f56988773261b33b66e316ae45b (presubmit successful).