[darcs-devel] darcs patch: Added "darcs show repo" command to display repository ...

[Kevin Quick]

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

- -----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

I've applied most of Eric's comments... thanks for the review.  I am  
sending a new patch shortly with those amendments.

Issues I didn't simply change:

>> +-- labelled strings: labels are right-aligned at 14 characters;
>> +-- subsequent lines in multi-line output are indented accordingly.
>> +showInfoUsr :: ShowInfo
>> +showInfoUsr t i = (replicate (14 - length(t)) ' ') ++ t ++ ": " ++
>> +                  (concat $ intersperse ('\n' : (replicate 16 '  
>> ')) $ lines i) ++ "\n"
>
> First, I'm not so keen on the fancy right-aligning code.  I realise  
> that
> it can be helpful for human readibility, but I wonder if we can find
> something that's a little bit friendlier to future changes.  For  
> example,
> maybe we could just use a tab character?  The output is not quite as
> nice-looking, but the underlying code is simpler.  I guess it  
> depends on
> whether you're more interested in people or third party tools.

I tend to like the right-aligned labels and vertical alignment of  
values because I do think it makes it more readable (my primary goal  
is human interaction, although maintaining regularity of output for  
tools is a close second).  I'm willing to cede to the group's wishes  
on this issue, but I'd prefer something like the current  
implementation for readability.  For this same reason, I prefer to  
keep the capitalization of the labels, even though it requires an  
extra function step for producing XML tags.

I hesitate to use tabs since it still requires counting and insertion  
of variable numbers of tabs, and looks less table-like.

The code is a bit more complicated, but it's pretty self-contained.   
I could extract the above to something more indicative of purpose  
like "rightAlign", but since it only appears in one place, that  
seemed superfluous.

Automated tools can still process this without much difficulty...  
just skip any leading whitespace and use the colon as a separator  
between label and value.  Or use the XML output format.

> Second, how about simplifying that bottom bit to
>
>   (unlines . map pad . lines $ i)
>   where pad x = replicate 16 ' ' ++ x

This doesn't allow the outdented label on the first line.  Adding  
that capability seems to get back to a similar complexity as the  
original in my attempts.

> Finally, I get the impression that you don't actually use this padding
> functionality (you seem to use ', ' to fit everything on one line)

Actually, it is the primary padding functionality.   There are some  
labelled values which can have multiple values (e.g. caches and  
repoformats) where I use commas within the line to separate the  
values (and further vertical bars between multiple elements of a  
repoformat).  I could use multi-line labelling instead.

> Finally, I think this is a good fit for do notation :-) I know you're
> not a big fan, but do-notation is also more darcs-like.  Hooray for
> mindless conformism.

No problem.  I don't want to be different just for the sake of being  
different.. maintaining confirmity frequently helps maintainability.   
This is mostly a case of personal blinders regarding my own natural  
coding style.  I've changed this, and please feel free to make  
further suggestions of this type.

>> +showRepoPrefs :: PutInfo -> IO ()
>> +showRepoPrefs out =
>> +    get_preflist "prefs" >>= mapM_ (uncurry out . (\(p,v) -> (p+ 
>> +" Pref",v)) . break (==' '))
>> +    >> get_preflist "author" >>= out "Author" . unlines
>> +    >> get_preflist "defaultrepo" >>= out "Default Remote" . unlines
>
> Here's how I might have rewritten this
>
> showRepoPrefs :: PutInfo -> IO ()
> showRepoPrefs out =
>  do get_preflist "prefs"  >>= mapM_ prefOut
>     get_preflist "author" >>= out "Author" . unlines
>     get_preflist "defaultrepo" >>= out "Default Remote" . unlines
>  where
>     prefOut pv = case break isSpace pv of
>                  (p,v) -> out (p ++ " Pref") (dropWhile isSpace v)

I took the first two suggestions: using prefOut and dropWhile.   I  
didn't take the third: using "case F of G" in place of functional  
composition "G .  F" because I think it's misleading: when I read  
"case", I expect to see a choice being made and it's a readability  
hiccup for me when it is used as a compositional idiom because I  
wonder what/where the other options are.

>> +showRepoMOTD :: PutInfo -> Repository -> IO ()
>> +showRepoMOTD out (Repo loc opts _ _) =
>> +             fetchFilePS opts (loc++"/_darcs/prefs/motd") (MaxAge  
>> 600) `catchall` return nilPS
>> +             >>= out "MOTD" . unpackPS
>
> You might consider refactoring this with Darcs.Repository.Motd

Done.  Involved an update in Darcs.Repository.Motd to split get_motd  
out of show_motd.  As a side benefit, this lead me to create an  
optimization in show_motd: the old form would *always* read the motd  
file, and only then check for the Quiet flag to suppress output; my  
update checks the Quiet flag first and avoids the file read as well  
if that flag is set.

Thanks!
   -KQ

- -----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.7 (Darwin)

iD8DBQFHQ8f1t76lKrRL0ewRAma5AJ95QJxLx0NfWCe/6qbA/h7qhldj1ACfePIO
6VHie+r2I3SXa4csp2NwLZc=
=GYwh
- -----END PGP SIGNATURE-----
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.7 (Darwin)

iD8DBQFHQ8gAt76lKrRL0ewRAnhXAJ9JGH7a7Du9UCiSvv0iBtWStkRLwgCcDL9o
d1IjzeR5wU6vVtXwFVskFjM=
=uvj/
-----END PGP SIGNATURE-----