On Sun, Dec 21, 2008 at 00:55:52 -0800, Trent W. Buck wrote: > Finished version of my work from yesterday.
Applied, thanks! > Sun Dec 21 19:19:34 EST 2008 Trent W. Buck <[email protected]> > * resolve issue948: rewrite darcsman. > > Significant changes are: > > - Avoid duplicating groups from TheCommands. > - Due to growing command_helps, list commands in SYNOPSIS. > - Use subsections (.SS) for groups. > - Include (with fancy markup!) command arguments. > - Include darcs help --match. > - Copy-and-paste description from darcs.cabal. > - Remove AUTHORS section as suggested by man-pages(7). > - Declare my copyright. resolve issue948: rewrite darcsman. ----------------------------------- I'm giving this only a cursory review. It might be good to try hlinting your code (hey maybe we should put hlint in http://wiki.darcs.net/DarcsWiki/DeveloperGettingStarted ) as well. > hunk ./src/darcsman.hs 36 > - -main = man (extract_commands command_control_list) > +main = mapM_ putStrLn ls Not that this makes a difference, but I slightly prefer putStr (unlines ls) to this because it minimises the IO footprint in our code (this is strictly in terms of code readibility, and is really really minor) > + [".SH DESCRIPTION", > + -- FIXME: this is copy-and-pasted from darcs.cabal, so > + -- it'll get out of date as people forget to maintain > + -- both in sync. You might be able to find a way to ask cabal to give you the contents of the cabal description. This may require a runhaskell Setup configure (or cabal configure), on the other hand, but it's worth thinking about for the future when we have blessed cabal as the official install method. > +-- | A synopsis line for each command. Uses 'foldl' because it is > +-- necessary to avoid blank lines from Hidden_commands, as groff > +-- translates them into annoying vertical padding (unlike TeX). I'm not sure what you mean by the 'uses foldl' comment. Perhaps a more stable phrasing might be just to say that we ignore blank lines from hidden commands (as groff translates them, etc) without mentioning the implementation details (uses foldl), because doing so may accidentally implies other things (e.g. uses foldl and not foldr) > + acc ++ (concatMap > + (render ((command_name c) ++ " ")) (render (command_name c ++ " ")) would be fine here or (render $ command_name c ++ " ") > hunk ./src/darcsman.hs 138 > +mangle_args :: String -> String > +mangle_args s = > + ".RI " ++ (unwords $ map show (groupBy cmp $ map toLower $ gank s)) > + where cmp x y = not $ xor (isAlphaNum x) (isAlphaNum y) > + xor x y = (x && not y) || (y && not x) > + gank (' ':'o':'r':' ':xs) = '|' : (gank xs) > + gank (x:xs) = x : (gank xs) > + gank [] = [] I suspect this could simplified somewhat. First, I'm guessing here that you're using show to add quotation marks around the strings, in which case, it may be more transparent if you made a binding quote = show. But if not, there may be some other simplifications we can do there. Second, in the groupBy logic, it seems like you're trying to clump the text into alphanumeric vs non-alphanumeric sections, eg. hello||bar-quux+toto = [ "hello", "||", "bar", "-", "quux", "+", "toto" ] Is it just the '|' inserted by gank (which translates the word 'or' into '|') that you're trying to do this for, or all non-alphanumeric characters? Maybe go ahead and implement xnor if you're going to define xor anyway? Would the on function be useful? Something like groupBy ((==) `on` isAlphaNum) -- Eric Kow <http://www.nltg.brighton.ac.uk/home/Eric.Kow> PGP Key ID: 08AC04F9
signature.asc
Description: Digital signature
_______________________________________________ darcs-users mailing list [email protected] http://lists.osuosl.org/mailman/listinfo/darcs-users
