Hi Don, On Sat, Dec 16, 2006 at 07:03:18PM +1100, Donald Bruce Stewart wrote:
Ok, I really want to push forwards the effort to add a nice popen to base. Here's my first effort at a minimal clean interface, that we might be proud to demonstrate in a tutorial ;)
That would be great! Some comments:
readProcess cmd args input = C.handle (return . handler) $ do
(inh,outh,errh,pid) <- runInteractiveProcess cmd args Nothing Nothing
-- fork off a thread to start consuming the output output <- hGetContents outh outMVar <- newEmptyMVar forkIO $ C.evaluate (length output) >> putMVar outMVar ()
outMVar isn't needed, is it? Just putting the "hClose outh" in place of the putMVar should work AFAICS. The only difference is that it wouldn't guarantee that the handle will be closed by the time the main functions is finished, but maybe with the finite number of file handles we have that is worth spending an MVar for.
-- now write and flush any input when (not (null input)) $ hPutStr inh input
unless (null input) $ hPutStr inh input
hClose inh -- done with stdin hClose errh -- ignore stderr
This will cause the command to fail if it tries to write to stderr (and checks the result of the write). Doing the same thing with stderr that you do with stdout would work.
-- wait on the output takeMVar outMVar hClose outh
-- wait on the process ex <- C.catch (waitForProcess pid) (\_ -> return ExitSuccess)
Catching all exceptions and treating them as success doesn't seem right? Why not just let them propagate?
return $ case ex of ExitSuccess -> Right output ExitFailure _ -> Left ex
where handler (C.ExitException e) = Left e handler e = Left (ExitFailure 1)
Again, why not let the exceptions propagate? If you do want to catch everything and return it as a value then I think you should allow the possibility to return an Exception. Thanks Ian