search

dart-lang / sdk

The Dart SDK, including the VM, JS and Wasm compilers, analysis, core libraries, and more.
https://dart.dev
BSD 3-Clause "New" or "Revised" License
10.09k stars 1.56k forks source link

BytesBuilder should probably be a Sink #28316

Open mraleph opened 7 years ago

mraleph commented 7 years ago

I can see a lot of examples on the web hand-rolling sinks on top of BytesBuilder. It would be much easier if BytesBuilder actually was a sink.

/cc @lrhn @floitschG @mkustermann

lrhn commented 7 years ago

True, it probably could be a Sink<List<int>>. Not that the close method would do anything useful, but still, it's definitely doable.

floitschG commented 7 years ago

Would a Sink be enough, or do you see wrappers for IOSink? (in which case there would be an addStream too).

mraleph commented 7 years ago

I think it needs to be a StreamConsumer to be ultimately useful.

lrhn commented 7 years ago

I think it would be too much to make it a StreamConsumer. It is a simple class that has one job. Trying to make it do more jobs at the same time is just over-complicating things (and failing at separation of concerns). Even making it a Sink is suspicious - why is this a Sink and List isn't?

I'd rather have good wrappers that can adapt something with an add method to be a Sink, and any Sink to a StreamConsumer. Adding close method to BytesBuilder makes no sense on its own - should you call it or not?

floitschG commented 7 years ago

Maybe, a better option would be to have something like:

var builder = new BytesBuilder();
var sink = new IOSink.wrapping(builder);
lrhn commented 7 years ago

I think that would be better. Then the only class worrying about the IOSink interface is the IOSink class, not the unrelated BytesBuilder.

It would be nice if the IOSink could wrap other objects too, but since the only relevant method on BytesBuilder is add(List<int>), there probably isn't any common interface of things to wrap

peter-ahe-google commented 7 years ago

What about moving it to dart:typed_data?