convert: new LineSplitter, (io: Deprecate LineTransformer)

convert: new LineSplitter, (io: Deprecate LineTransformer)

R=floitsch@google.com

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

[kevmoo-old, 2013-08-04 02:47:00]

I've wanted this out of dart:io for a long time

[kevmoo-old, 2013-08-04 02:50:51]

FYI

https://codereview.chromium.org/22070003/diff/3001/sdk/lib/io/string_transfor...
File sdk/lib/io/string_transformer.dart (right):

https://codereview.chromium.org/22070003/diff/3001/sdk/lib/io/string_transfor...
sdk/lib/io/string_transformer.dart:266: class LineTransformer implements
StreamTransformer<String, String> {
I've changed the base class from StreamEventTransformer to StreamTransformer.

Certainly an API breaking-change, although nothing is test suite seems to break.

[kevmoo-old, 2013-08-05 19:26:33]

Fixed reviewers

[floitsch, 2013-08-07 12:58:26]

LGTM.
Still some TODOs left, but they can be done later.

See for example
https://chromiumcodereview.appspot.com/17580014/diff/5001/sdk/lib/convert/lin...
for an old prototype.
Again: should be done in a separate CL.

https://codereview.chromium.org/22070003/diff/20001/sdk/lib/convert/line_spli...
File sdk/lib/convert/line_splitter.dart (right):

https://codereview.chromium.org/22070003/diff/20001/sdk/lib/convert/line_spli...
sdk/lib/convert/line_splitter.dart:19: ChunkedConversionSink
startChunkedConversion(ChunkedConversionSink<String> sink) {
80 chars.

https://codereview.chromium.org/22070003/diff/20001/sdk/lib/convert/line_spli...
sdk/lib/convert/line_splitter.dart:19: ChunkedConversionSink
startChunkedConversion(ChunkedConversionSink<String> sink) {
Change return type to StringConversionSink

https://codereview.chromium.org/22070003/diff/20001/sdk/lib/convert/line_spli...
sdk/lib/convert/line_splitter.dart:53: static String _addSlice(String chunk, int
start, int end, bool isLast, void adder(String val)) {
80 chars.

No need to make this method static. By making it static we have to closurize
_sink.add.

https://codereview.chromium.org/22070003/diff/20001/sdk/lib/convert/line_spli...
sdk/lib/convert/line_splitter.dart:88: }
Add TODO(floitsch): deal with utf8.

https://codereview.chromium.org/22070003/diff/20001/sdk/lib/io/string_transfo...
File sdk/lib/io/string_transformer.dart (right):

https://codereview.chromium.org/22070003/diff/20001/sdk/lib/io/string_transfo...
sdk/lib/io/string_transformer.dart:263: * Use [LineSplitter] from `dart:convert`
instead.
If you feel like it (if not I can do it later):
Add removal date, let's say 3 weeks from now.
Add it in comment, and update the comment in the internal _collection_dev
library (sdk/lib/_collection_dev/iterable.dart). Reassign TODO there to me.

https://codereview.chromium.org/22070003/diff/20001/tests/lib/convert/line_sp...
File tests/lib/convert/line_splitter_test.dart (right):

https://codereview.chromium.org/22070003/diff/20001/tests/lib/convert/line_sp...
tests/lib/convert/line_splitter_test.dart:23: var inputs = const ['line1',
'line2', 'long line 3', ' this is line 4 ', 'l5'];
80chars.

https://codereview.chromium.org/22070003/diff/20001/tests/lib/convert/line_sp...
tests/lib/convert/line_splitter_test.dart:44: ChunkedConversionSink byteSink =
Rename byteSink to stringSink.

https://codereview.chromium.org/22070003/diff/20001/tests/lib/convert/line_sp...
tests/lib/convert/line_splitter_test.dart:46: var stringConversionSink = new
LineSplitter().startChunkedConversion(byteSink);
80chars.

https://codereview.chromium.org/22070003/diff/20001/tests/lib/convert/line_sp...
tests/lib/convert/line_splitter_test.dart:107: controller.add("Line".codeUnits);
Add comment that UTF8 is ok, because the string is only ASCII.

https://codereview.chromium.org/22070003/diff/20001/tests/lib/convert/line_sp...
tests/lib/convert/line_splitter_test.dart:173:
controller.add("Line1\nLine2\r\nLine3\rLi".codeUnits);
Add comment that "codeUnits" is ok, because the string only contains ASCII.

[kevmoo-old, 2013-08-07 17:13:41]

A few more things to do.

Please see my comment on _addSlice being static: I use it from two classes

https://codereview.chromium.org/22070003/diff/20001/sdk/lib/convert/line_spli...
File sdk/lib/convert/line_splitter.dart (right):

https://codereview.chromium.org/22070003/diff/20001/sdk/lib/convert/line_spli...
sdk/lib/convert/line_splitter.dart:19: ChunkedConversionSink
startChunkedConversion(ChunkedConversionSink<String> sink) {
On 2013/08/07 12:58:26, floitsch wrote:
> 80 chars.

Done.

https://codereview.chromium.org/22070003/diff/20001/sdk/lib/convert/line_spli...
sdk/lib/convert/line_splitter.dart:19: ChunkedConversionSink
startChunkedConversion(ChunkedConversionSink<String> sink) {
On 2013/08/07 12:58:26, floitsch wrote:
> Change return type to StringConversionSink

Done.

https://codereview.chromium.org/22070003/diff/20001/sdk/lib/convert/line_spli...
sdk/lib/convert/line_splitter.dart:53: static String _addSlice(String chunk, int
start, int end, bool isLast, void adder(String val)) {
On 2013/08/07 12:58:26, floitsch wrote:
> 80 chars.
> 
> No need to make this method static. By making it static we have to closurize
> _sink.add.

I use _addSlice in LineSplitter, hence the static. Thoughts?

https://codereview.chromium.org/22070003/diff/20001/sdk/lib/convert/line_spli...
sdk/lib/convert/line_splitter.dart:88: }
On 2013/08/07 12:58:26, floitsch wrote:
> Add TODO(floitsch): deal with utf8.

Done.

https://codereview.chromium.org/22070003/diff/20001/tests/lib/convert/line_sp...
File tests/lib/convert/line_splitter_test.dart (right):

https://codereview.chromium.org/22070003/diff/20001/tests/lib/convert/line_sp...
tests/lib/convert/line_splitter_test.dart:23: var inputs = const ['line1',
'line2', 'long line 3', ' this is line 4 ', 'l5'];
On 2013/08/07 12:58:26, floitsch wrote:
> 80chars.

Done.

https://codereview.chromium.org/22070003/diff/20001/tests/lib/convert/line_sp...
tests/lib/convert/line_splitter_test.dart:44: ChunkedConversionSink byteSink =
On 2013/08/07 12:58:26, floitsch wrote:
> Rename byteSink to stringSink.

Done.

https://codereview.chromium.org/22070003/diff/20001/tests/lib/convert/line_sp...
tests/lib/convert/line_splitter_test.dart:46: var stringConversionSink = new
LineSplitter().startChunkedConversion(byteSink);
On 2013/08/07 12:58:26, floitsch wrote:
> 80chars.

Done.

https://codereview.chromium.org/22070003/diff/20001/tests/lib/convert/line_sp...
tests/lib/convert/line_splitter_test.dart:107: controller.add("Line".codeUnits);
On 2013/08/07 12:58:26, floitsch wrote:
> Add comment that UTF8 is ok, because the string is only ASCII.

Done.

https://codereview.chromium.org/22070003/diff/20001/tests/lib/convert/line_sp...
tests/lib/convert/line_splitter_test.dart:173:
controller.add("Line1\nLine2\r\nLine3\rLi".codeUnits);
On 2013/08/07 12:58:26, floitsch wrote:
> Add comment that "codeUnits" is ok, because the string only contains ASCII.

Done.

[kevmoo-old, 2013-08-07 19:41:08]

https://codereview.chromium.org/22070003/diff/20001/sdk/lib/io/string_transfo...
File sdk/lib/io/string_transformer.dart (right):

https://codereview.chromium.org/22070003/diff/20001/sdk/lib/io/string_transfo...
sdk/lib/io/string_transformer.dart:263: * Use [LineSplitter] from `dart:convert`
instead.
On 2013/08/07 12:58:26, floitsch wrote:
> If you feel like it (if not I can do it later):
> Add removal date, let's say 3 weeks from now.
> Add it in comment, and update the comment in the internal _collection_dev
> library (sdk/lib/_collection_dev/iterable.dart). Reassign TODO there to me.

Not sure what to add to _collection_dev/iterable.dart. Help?

[kevmoo-old, 2013-08-07 20:25:50]

Committed patchset #9 manually as r25899 (presubmit successful).

[Søren Gjesse, 2013-08-12 12:57:27]

Drive by comments.

https://codereview.chromium.org/22070003/diff/30001/sdk/lib/convert/line_spli...
File sdk/lib/convert/line_splitter.dart (right):

https://codereview.chromium.org/22070003/diff/30001/sdk/lib/convert/line_spli...
sdk/lib/convert/line_splitter.dart:41: if(_carry != null) {
Space between if and (.

https://codereview.chromium.org/22070003/diff/30001/sdk/lib/convert/line_spli...
sdk/lib/convert/line_splitter.dart:48: if(isLast) _sink.close();
Ditto.

https://codereview.chromium.org/22070003/diff/30001/sdk/lib/convert/line_spli...
sdk/lib/convert/line_splitter.dart:55: static String _addSlice(String chunk, int
start, int end, bool isLast, void adder(String val)) {
Long line.

https://codereview.chromium.org/22070003/diff/30001/sdk/lib/convert/line_spli...
sdk/lib/convert/line_splitter.dart:81: if(isLast) {
Space between if and (.

https://codereview.chromium.org/22070003/diff/30001/tests/lib/convert/line_sp...
File tests/lib/convert/line_splitter_test.dart (right):

https://codereview.chromium.org/22070003/diff/30001/tests/lib/convert/line_sp...
tests/lib/convert/line_splitter_test.dart:176:
controller.add("ne4\n".codeUnits);
Looks as if input break between \r and \n is not tested. Somethink like:

controller.add("Line5\r");
controller.add("\nLine6\n");