xtendo-org / rawfilepath

Haskell library for encoding-free interaction with Unix system. Use RawFilePath (ByteString) instead of FilePath (String)
Other
17 stars 3 forks source link

Proposal: Add more generalized version of `proc` #4

Closed xnuk closed 4 years ago

xnuk commented 7 years ago

With proc', you can use like this:

main = do
    p <- startProcess $ proc' "echo" ["hello"] :: IO (Process NoStream CreatePipe NoStream)
    result <- B.hGetContents (processStdout p)
    _ <- waitForProcess p

    print (result == "hello\n")

Or:

main = do
    p <- startProcess $ proc' "echo" ["hello"] `setStderr` NoStream `setStdin` NoStream
    result <- B.hGetContents (processStdout p)
    --                          ^ This determines stdout is `CreatePipe`
    _ <- waitForProcess p

    print (result == "hello\n")

Additional changes

xtendo-org commented 4 years ago

Thank you for sending us this Pull Request. I've given this some time and thought.

In your second example, processStdout p would automatically determine that stdout is CreatePipe. But to gain that advantage, you need to explicitly set stderr and stdin.

Compare your example:

    p <- startProcess $ proc' "echo" ["hello"] `setStderr` NoStream `setStdin` NoStream
    result <- B.hGetContents (processStdout p)

with the original documentation example:

    p <- startProcess $ proc "echo" ["hello"] `setStdout` CreatePipe
    result <- B.hGetContents (processStdout p)

The original version is actually shorter because, with the former, we have to manually set NoStream for the two unused streams. Otherwise it would be a type ambiguity error.

Indeed, in your version, a code that uses all three of processStdxxx would have a slightly shorter code, and Haskell would "intelligently" determine the stream types without any type signatures or setStdxxx functions. So saving the effort of setStdxxx seems to be the key benefit of this Pull Request. But it also means that as soon as the programmer removes a processStdxxx from the function definition (because e.g. it became no longer necessary), the code that used to compile fine would stop working and start raising a type ambiguity error. The "saving" benefit looks too small to introduce a new function that does almost the same thing as the original.

If the issue is having to use setStdxxx thrice for opening a process with all streams as pipes, we can simply add a convenience function that sets all three streams to CreatePipe and name it to something like procAllPipes. I still think the amount of saving is too small (only a few keystrokes), but if enough people want it, I'm not against adding convenience functions in general.

Your Pull Request is well coded, documented, and styled. I feel grateful for the time and effort you took for making this, and I'm sorry to say that I'm not in favor of merging this. Please feel free to reply though, if you have any further opinions or comments.