Ganesh Sittampalam <[email protected]> added the comment: These are two independent patches and would probably be easier to handle as two separate submissions.
> - let matcher = get_matcher opts > + let matcher = fromMaybe (error $ "show_contents_cmd, no matcher: " ++ (show opts)) (nonrange_matcher opts) For the first one, "produce better error if matcher can't be found", anything is clearly better than a fromJust error. But given that this is reporting on a user error (invalid command-line argument), I think something that doesn't refer to the internal darcs function would be more appropriate. Also it would be useful if the patch comments made it clear what triggers the error message. I think the second patch is actually wrong, as it moves some operations outside the "withRepository" wrapper. I also don't (personally) think that using a reverse pipeline is particularly helpful in this situation, though opinion may obviously differ on that. > -show_index_cmd opts _ = withRepository opts $- \repo -> do > - readIndex repo >>= updateIndex >>= dump opts > +show_index_cmd opts _ = dump opts =<< updateIndex =<< (withRepository opts $- readIndex) > -show_pristine_cmd opts _ = withRepository opts $- \repo -> do > - readRecorded repo >>= dump opts > +show_pristine_cmd opts _ = dump opts =<< (withRepository opts $- > readRecorded ) So I'd be happy to apply the first one with an improved error message and a bit more explanation about what triggers it, but I'd rather just drop the second one. ---------- assignedto: -> ganesh nosy: +ganesh status: needs-review -> amend-requested __________________________________ Darcs bug tracker <[email protected]> <http://bugs.darcs.net/patch103> __________________________________ _______________________________________________ darcs-users mailing list [email protected] http://lists.osuosl.org/mailman/listinfo/darcs-users
