michaelt / text-pipes

Text pipes
BSD 3-Clause "New" or "Revised" License
10 stars 13 forks source link

Add readFileLines function #18

Closed sid-kap closed 8 years ago

sid-kap commented 8 years ago

Reads files, separated by newlines.

Does this make good use of buffering?

michaelt commented 8 years ago

I think we should ask @Gabriel439 what he thinks of this.

Of course one wants this operation all the time, and a similar stdinLn writeFileLn and so on. The operations can be easily enough constructed with the materials the the different pipes packages, but it is a bit of a mental strain where one is doing trivial local file IO.

The trouble is that they work somewhat against the point of view of pipes-text which is of an undifferentiated character stream where chunking is not semantically significant. But I wonder if the best solution isn't a separate module Pipes.Prelude.Text that replaces the ABC IO in Pipes.Prelude with corresponding Text operations, and perhaps re-exports the rest of Pipes.Prelude. It would still be ABC IO since it accumulates potentially infinitely long lines and thus needs careful commentary and so on. It could of course be a different package, but this one already incurs the text dependency so it a convenient place to put such operations. It may be that other sensible operations come to mind once one takes the line-based understanding of Producer Text m r.

Gabriella439 commented 8 years ago

I think it's fine as long as there is a warning in the documentation explaining that this does not handle really long lines well (and a link to a reference explaining how one might handle those)

michaelt commented 8 years ago

So @sid-kap maybe the best plan is to have a separate stanza in the haddocks for line based functions, maybe these

stdinLn
stdoutLn
writeFileLn
readFileLn

and presumably

toHandleLn
fromHandleLn

with the obvious commentary.

If that makes sense of course it might make sense to get rid of the existing operations since they sort of mix the official point of view with concessions to useability. On the other hand, maybe the systematic contrast between the two stanzas of operations would help make the relevant tutorial point about line lengths etc. and the general point of view of pipes-bytestring + pipes-text

If that's right we should rename readFileLine to readFileLn for starters.

sid-kap commented 8 years ago

Sure, that sounds like a good idea. Are you still in favor of making a Pipes.Text.Prelude (or Pipes.Prelude.Text) module and putting these there?

Also, what does "ABC IO" mean?

michaelt commented 8 years ago

No, I think this plan is good, on reflection. Thanks for raising the issue, which is obviously a good one.

michaelt commented 8 years ago

Oh, if you just rename it, I can apply this patch. If you like you can implement the others, or we can divide the labor, or I can write them, or whatever you please. We should cross-check that we are in correspondence with pipes prelude and pipes safe, as I think you already did in this case. It occurs to me this will mean that we have both a stdinLn and stdinLn', if I remember.

sid-kap commented 8 years ago

I think I'll just submit this function, and let you guys implement the others.

Thanks for taking a look at this issue! I really appreciate it.

michaelt commented 8 years ago

Thanks @sid-kap !

michaelt commented 8 years ago

@sid-kap @Gabriel439 @GregorySchwartz I went for a separate module once I saw what it all looked like https://github.com/michaelt/text-pipes/blob/master/Pipes/Prelude/Text.hs I hope the explanatory material there is intelligible and gives sufficient warning about the limitations of line-based text io.

sid-kap commented 8 years ago

@michaelt Thanks so much for doing this!