[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