[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] Re: [Xen-devel] [PATCH 13 of 17] docs: generate an index for the html output
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
|
Lists.xenproject.org is hosted with RackSpace, monitoring our |