[darcs-devel] [patch1017] Resolve issue2250: variable tabbing in usageHelper

[Owen Stephens]

New submission from Owen Stephens <darcs at owenstephens.co.uk>:

Hi bsrkaditya!

> 1 patch for repository http://darcs.net/screened:
> 
> Mon Feb  4 17:29:05 IST 2013  bsrkaditya at 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.

----------
assignedto:  -> bsrkaditya
nosy: +bsrkaditya
status: needs-screening -> followup-requested

__________________________________
Darcs bug tracker <bugs at darcs.net>
<http://bugs.darcs.net/patch1017>
__________________________________