Hi bsrkaditya!
> 1 patch for repository http://darcs.net/screened:
>
> Mon Feb 4 17:29:05 IST 2013 bsrkaditya@gmail.com
> * Resolve issue2250: variable tabbing in usageHelper
It'd be good to say how you fix the issue, perhaps:
"Resolve issue2250: tabbing in usageHelper - pad by max length of
command name"
> usageHelper :: [CommandControl] -> String
> -usageHelper [] = ""
> -usageHelper (HiddenCommand _ : cs) = usageHelper cs
> -usageHelper (CommandData c : cs) = " " ++ padSpaces (commandName c) 15
> +usageHelper xs = usageHelper' (maximum.f $ xs) xs
> + where f (CommandData c: cs) = ((+2).length.commandName $ c): f cs
> + f (_: cs) = f cs
> + f [] = [15]
> +
`f' here is a poor choice for a function name - it's not clear what it's
doing
(short named helpers are fine, if their purpose is obvious from context)
I'm also going to be picky and suggest spacing around the . operator -
as per
the style guidelines.
Also, I don't like the f [] = [15] idiom - Maybe Int as a result type, and
fromMaybe would be better - it doesn't currently make its intention clear.
> +usageHelper' :: Int -> [CommandControl] -> String
> +usageHelper' _ [] = ""
> +usageHelper' x (HiddenCommand _ : cs) = usageHelper' x cs
> +usageHelper' x (CommandData c : cs) = " " ++ padSpaces (commandName c) x
> ++ chompNewline
(commandDescription c)
> hunk ./src/Darcs/UI/Commands.hs 234
> - ++ "\n" ++ usageHelper cs
> -usageHelper (GroupName n:cs) = "\n" ++ n ++ "\n" ++ usageHelper cs
> + ++ "\n" ++ usageHelper' x cs
> +usageHelper' x (GroupName n:cs) = "\n" ++ n ++ "\n" ++ usageHelper' x cs
Consistency in style please - here you've not used space around the
binary `:',
whereas in the other cases of this function you have.
> - commandDescription = " Report patch-index status",
> + commandDescription = "Report patch-index status",
This should really be a separate patch, but that's probably being a bit
picky
:-) it's "come out of nowhere" w.r.t. the bug/patch titles.
Thanks for the patch!
Owen.
|