Ganesh Sittampalam <[email protected]> added the comment: In general I think this looks good, some detailed comments/requests for changes/further explanation below.
As you say this is a baby step and noone should rely on the precise signature of the resulting API functions, though we should certainly aim to always provide this kind of functionality in some form. > [in show files, break out manifestCmd' helper function with result type IO [FilePath], as baby step towards a useful darcs api > [email protected]**20100102140103 > Ignore-this: 5290a1b50a4a23a961138920de7bc6b4 > ] This patch has a spurious whitespace change at the end. Also, I'm not convinced by the naming of "filesCmd'" for an exported function intended to be in the API. I'd suggest renaming things appropriately so this can be "filesCmd" or "filesCommand". > [simplify output subfunction > [email protected]**20100102161308 > Ignore-this: b32d46fdfa02ee74425966747a711226 > ] > - where output_null name = do { putStr name ; putChar '\0' } > - output = if NullFlag `elem` opts then output_null else putStrLn > + where output name = do putStr name > + putChar $ if NullFlag `elem` opts then '\0' else > '\n' Is this change correct on Windows? I suspect so but we should check. > [make monad pipeline explicit > [email protected]**20100102174548 > Ignore-this: 8f10d4403232fc96f55834833736403c > ] > - do x <- do pid <- runProcess c args Nothing Nothing (Just h) Nothing Nothing > - waitForProcess pid > + do x <- do waitForProcess =<< runProcess c args Nothing Nothing (Just h) Nothing Nothing > when (x == ExitFailure 127) $ In general this kind of change feels rather like churn, though I think in this particular case it might be a slight improvement. However you have left in an unnecessary "do". > [make slurpPristine use absolute paths (no more need to wrap in getCurrrentDirectory/withCurrentDirectory) > [email protected]**20100102175426 > Ignore-this: 9e0d47590c4b92d9c6c774730136113e > ] I'm not convinced this patch is correct, since mmap_slurp itself looks at the current directory. I definitely support the intention though. Could you provide more justification of why it's ok? ---------- status: needs-review -> review-in-progress __________________________________ Darcs bug tracker <[email protected]> <http://bugs.darcs.net/patch127> __________________________________ _______________________________________________ darcs-users mailing list [email protected] http://lists.osuosl.org/mailman/listinfo/darcs-users
