[darcs-users] darcs patch: Tweak punctuation in "darcs help --match". (and 1 more)

Trent W. Buck trentbuck at gmail.com
Sun Dec 21 14:24:08 UTC 2008


On Sun, Dec 21, 2008 at 01:40:54PM +0000, Eric Kow wrote:
> 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.

Unfortunately, hlint needs base 4 (i.e. GHC 6.10) and I only have easy
access to 6.8.

> > 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)

Agreed.

> > +     [".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.

Well, it doesn't necessarily have to come from darcs.cabal; the same
text is also in doc/index.html and a couple of other places (like
debian/control).  I don't think there's an easy solution to avoid
copy-and-pasting between all those places, but I am certainly open to
suggestions.

> > +-- | 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

Sure thing.  I don't remember precedence order (I can't even use an
infix pocket calculator; it has to be postfix!), so I just kept adding
parens until GHC stopped complaining; I forgot to go through and clean
them up again.

> > 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

Yep; mainly because it was shorter than

    where quote x = "\"" ++ x ++ "\""

> Second, in the groupBy logic, it seems like you're trying to clump the
> text into alphanumeric vs non-alphanumeric sections, eg.

Yep; that's the "clever" part; because I need to go from

    <FOO> [<BAR or BAZ>]...

to

    .RI < foo "> [<" bar | baz >]...

which in TeX syntax would be

    <\emph{foo}> [<\emph{bar}|\emph{baz}>]...

the code is (ab)using the knowledge that the option names are all
words.  It does make one mistake: the arguments to darcs initialize.

roff note: the .RI directive means "make odd words on this line roman
(normal), and even words italicized, and remove space between words"
where a "word" can contain whitespace if surrounded by "double quotes".

There's an implied assumption that all argument strings start with a
nonalnum (hence roman) character, e.g. [ or <.

> Maybe go ahead and implement xnor if you're going to define xor anyway?

Shrug.

> Would the on function be useful?  Something like groupBy ((==) `on`
> isAlphaNum)

I didn't think `on` had the correct semantics.  IIRC they are:

    prop_X True True = True
    prop_X True False = False
    prop_X False True = False
    prop_X False False = True

such that

    prop_Y "<foo> [<bar|baz>]..." = ["<","foo","> [<","bar","|","baz",">]..."]


More information about the darcs-users mailing list