ocaml-multicore / eio

Effects-based direct-style IO for multicore OCaml
Other
548 stars 66 forks source link

Add Flow.read_all #596

Closed SGrondin closed 11 months ago

SGrondin commented 1 year ago

This is really basic, but it's something that I've found useful to have.

When I first started experimenting with Eio, the only slight pain point I encountered at the very beginning was trying to understand how Flows, Files, Fs, Flow.Buf_read, etc, worked together. Having this simple function would have helped me understand it all.

It can also be useful for debugging.

SGrondin commented 11 months ago

I used Buffer to avoid the dependency issue since this function isn't meant to be ultra performant. I'll give it another shot this weekend and see what I can do.

SGrondin commented 11 months ago

@talex5 I rewrote it using Buf_read. I tried a few different approaches to break the cyclic dependency and I think this is the simplest way. If a better way exists, feel free to close this PR.

Edit: I'm not convinced the performance is worth the extra complexity if this function is not intended as more than a debugging or discovery or small scale tool.

talex5 commented 11 months ago

I tried a few different approaches to break the cyclic dependency and I think this is the simplest way. If a better way exists, feel free to close this PR

A simpler way might be to add it in eio.ml, e.g.

module Buf_read = Buf_read
module Flow = struct
  include Flow
  let read_all = ...
end
SGrondin commented 11 months ago

Great idea, I like that a lot better.

EDIT: done. Much better 🎉