Re: [Xen-devel] [PATCH 13 of 17] docs: generate an index for the html output

[Ian Jackson <Ian.Jackson@xxxxxxxxxxxxx>]

Ian Campbell writes ("[Xen-devel] [PATCH 13 of 17] docs: generate an index for 
the html output"):
> docs: generate an index for the html output
> 
> nb: I'm not a Perl wizard...

The Perl looks reasonable in general, except that some of the style is
a bit odd.  perlstyle(1) is mostly a good guide.

In particular

> +sub write_file ($$) {
                 ^
vs.

> +sub make_page($$$) {
                ^
perlsub(1) suggests inculding the space in Perl sub prototypes.
(Multiple occurrences.)

> +    $l =~ s/.(html)$//g;

Why the capturing parens ?

> +    if ( $title eq "" )
> +    {

Brace should be on the same line.

> +     $title = $h1 = "Xen Documentation";

And indent should be 4, not 1.  (Multiple occurrences.)

> +    print STDERR "Writing: $file\n";
> +    write_file($file, $o);

Perhaps (i) the print should be to STDOUT (ii) it should be in
write_file ?

> +sub read_index
> +{

Missing prototype and bracket should be on same line.  To make the
prototype work you'll probably have to move the definition to before
the option parser call (or have a declaration).  Perhaps the main
program should be at the bottom.

> +     s/#.*$//;

This of course prevents link anchor texts in the inde including #,
which is probably an error which it would be nice to sort out now
rather than in the future when we'll have to read this script to make
it cope.

> +     m/([^\t]+)\t+(.*)/ or die;

This reliance on hard tabs will irritate many people.  You should use
\S and \s.  The filenames (the LHS) won't contain whitespace of
course.

Also you probably meant to anchor the pattern.  I would do something
like:

    s/^\s+//;
    s/\s+$//;
    next if m/^\#/;
    next unless m/\S/;
    m/^(\S+)\s+(\S.*)$/ or die;

> +for (@docs) { s,^${outdir}/,, }

This is not correct because $outdir is not a regular expression.  The
shortest way of doing this is indeed substr.

> +my $top = '';
> +
> +foreach my $od (sort { $a cmp $b } uniq map { dirname($_) } @docs) {
> +    my @d = (grep /^$od/, @docs);

Again, directory names are not regexps.

Do we really want an index per subdirectory ?

> +    if ( $#d == 0 and $d[0] eq "$od/index.html" )

$#d==0 is a rather odd way of putting it.  I would write @d==1.

> +     $top .= "<li><a href=\"${od}/index.html\">$od</a></li>\n";
> +     $top .= "<ul>\n";
> +     $top .= make_links($od,0,@d);
> +     $top .= "</ul>\n";

Maybe this wants a here document ?
        my $links = make_links blah blah;
        $top .= <<END;

Ian.

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxx
http://lists.xensource.com/xen-devel