Add sinks to provide synchronous access to events.

Add sinks to provide synchronous access to events.

These are useful when doing chunked conversion in a synchronous context.

Closes #2

R=lrn@google.com

Committed: https://github.com/dart-lang/convert/commit/cd3d6123a6ca69dd62c82a7f581c0d1579a466a8

[nweiz, 2016-04-21 20:37:46]

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

[Lasse Reichstein Nielsen, 2016-04-22 09:15:14]

lgtm

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

https://codereview.chromium.org/1911053002/diff/1/lib/src/accumulator_sink.da...
lib/src/accumulator_sink.dart:7: /// A sink that provides synchronous access to
all the [events] that have been
Drop the "synchronous".

https://codereview.chromium.org/1911053002/diff/1/lib/src/byte_accumulator_si...
File lib/src/byte_accumulator_sink.dart (right):

https://codereview.chromium.org/1911053002/diff/1/lib/src/byte_accumulator_si...
lib/src/byte_accumulator_sink.dart:10: /// A sink that provides synchronous
access to the concatenated bytes passed to
I don't think you need to say "synchronous" here. 
It just provides access to the bytes passed to it so far, and sinks are
generally synhronous (what they do with the input might not be, but the sink
itself is).

https://codereview.chromium.org/1911053002/diff/1/lib/src/byte_accumulator_si...
lib/src/byte_accumulator_sink.dart:15: /// This should not be modified, although
this restriction isn't enforced.
More precisely:

The returned `Uint8List` is viewing a shared buffer, so it should not be changed
and any bytes outside the view should not be accessed.

https://codereview.chromium.org/1911053002/diff/1/lib/src/byte_accumulator_si...
lib/src/byte_accumulator_sink.dart:17: final _buffer = new Uint8Buffer();
Empty line before

https://codereview.chromium.org/1911053002/diff/1/lib/src/byte_accumulator_si...
lib/src/byte_accumulator_sink.dart:20: bool get isClosed => _isClosed;
As usual I prefer the fields before the getters. It's hard to figure out the
internal state of the class when the fields are spread out between other
members.

https://codereview.chromium.org/1911053002/diff/1/lib/src/string_accumulator_...
File lib/src/string_accumulator_sink.dart (right):

https://codereview.chromium.org/1911053002/diff/1/lib/src/string_accumulator_...
lib/src/string_accumulator_sink.dart:7: /// A sink that provides synchronous
access to the concatenated strings passed
Again, no need for "synchronous".

[Lasse Reichstein Nielsen, 2016-04-22 09:17:33]

You can also refer to ChunkedConversionSink.withCallback for the case where you
just want the list of added events when the sink is closed.

[Lasse Reichstein Nielsen, 2016-04-22 09:18:51]

And maybe consider having a "clear" method on the sinks, so you can take the
events so far, and then remove them so they are not repeated the next time you
get the accumulated data.

[nweiz, 2016-04-22 21:19:36]

Code review changes

[nweiz, 2016-04-22 21:19:56]

> You can also refer to ChunkedConversionSink.withCallback
> for the case where you just want the list of added events
> when the sink is closed.

Done.

> And maybe consider having a "clear" method on the sinks,
> so you can take the events so far, and then remove them so
> they are not repeated the next time you get the accumulated
> data.

Done.

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

https://codereview.chromium.org/1911053002/diff/1/lib/src/accumulator_sink.da...
lib/src/accumulator_sink.dart:7: /// A sink that provides synchronous access to
all the [events] that have been
On 2016/04/22 09:15:13, Lasse Reichstein Nielsen wrote:
> Drop the "synchronous".

Done.

https://codereview.chromium.org/1911053002/diff/1/lib/src/byte_accumulator_si...
File lib/src/byte_accumulator_sink.dart (right):

https://codereview.chromium.org/1911053002/diff/1/lib/src/byte_accumulator_si...
lib/src/byte_accumulator_sink.dart:10: /// A sink that provides synchronous
access to the concatenated bytes passed to
On 2016/04/22 09:15:13, Lasse Reichstein Nielsen wrote:
> I don't think you need to say "synchronous" here. 
> It just provides access to the bytes passed to it so far, and sinks are
> generally synhronous (what they do with the input might not be, but the sink
> itself is).

Done.

https://codereview.chromium.org/1911053002/diff/1/lib/src/byte_accumulator_si...
lib/src/byte_accumulator_sink.dart:15: /// This should not be modified, although
this restriction isn't enforced.
On 2016/04/22 09:15:13, Lasse Reichstein Nielsen wrote:
> More precisely:
> 
> The returned `Uint8List` is viewing a shared buffer, so it should not be
changed
> and any bytes outside the view should not be accessed.

Done.

https://codereview.chromium.org/1911053002/diff/1/lib/src/byte_accumulator_si...
lib/src/byte_accumulator_sink.dart:17: final _buffer = new Uint8Buffer();
On 2016/04/22 09:15:13, Lasse Reichstein Nielsen wrote:
> Empty line before

Done.

https://codereview.chromium.org/1911053002/diff/1/lib/src/byte_accumulator_si...
lib/src/byte_accumulator_sink.dart:20: bool get isClosed => _isClosed;
On 2016/04/22 09:15:13, Lasse Reichstein Nielsen wrote:
> As usual I prefer the fields before the getters. It's hard to figure out the
> internal state of the class when the fields are spread out between other
> members.

I think of fields as a special case of getters, so it makes sense that they're
organized together. I also think of getters as important views into a class's
state; even if they're derived data, they're important enough to be made easily
accessible which means they're relevant to understanding the semantic state of
the object.

Also, when two members are tightly coupled (for example a getter that returns an
unmodifiable view of a field), I find it much clearer to visually group them as
well.

https://codereview.chromium.org/1911053002/diff/1/lib/src/string_accumulator_...
File lib/src/string_accumulator_sink.dart (right):

https://codereview.chromium.org/1911053002/diff/1/lib/src/string_accumulator_...
lib/src/string_accumulator_sink.dart:7: /// A sink that provides synchronous
access to the concatenated strings passed
On 2016/04/22 09:15:14, Lasse Reichstein Nielsen wrote:
> Again, no need for "synchronous".

Done.

[nweiz, 2016-04-22 21:20:08]

Description was changed from

==========
Add sinks to provide synchronous access to events.

These are useful when doing chunked conversion in a synchronous context.

Closes #2
==========

to

==========
Add sinks to provide synchronous access to events.

These are useful when doing chunked conversion in a synchronous context.

Closes #2

R=lrn@google.com

Committed:
https://github.com/dart-lang/convert/commit/cd3d6123a6ca69dd62c82a7f581c0d157...
==========

[nweiz, 2016-04-22 21:20:09]

Committed patchset #2 (id:20001) manually as
cd3d6123a6ca69dd62c82a7f581c0d1579a466a8 (presubmit successful).