Code review: code-dashboard-initial-20-Jan-2008

[GoogleCodeExporter]

We'll be getting a continuous build server soon, and this should help us
put together a dashboard showing test and coverage stats, task comments
from code, documentation, and variables useful for profiling.

Original issue reported on code.google.com by mikesamuel@gmail.com on 23 Jan 2008 at 10:31

[GoogleCodeExporter]

dashboard.pl

In general, you shouldn't use "my" variables in subs: use "our" instead.

die messages should say what the problem is, as well as the name of the
file/directory that is a problem.

Instead of repeating strings like "$dashboard_root/time-series/" assign them to
variables.

Otherwise LGTM.

Original comment by ben@links.org on 1 Feb 2008 at 11:49

[GoogleCodeExporter]

are you sure "our" is what you want?   "our" imports a global var into the 
current
lexical scope.  that's not usually what I want in a sub.

Original comment by felix8a on 1 Feb 2008 at 12:22

[GoogleCodeExporter]

I fixed all the die messages.
$  grep die dashboard/dashboard.pl
    die "output directory redeclared.  Was $OUTPUT_DIR" if defined($OUTPUT_DIR);
    die "build client redeclared.  Was $BUILD_CLIENT" if defined($BUILD_CLIENT);
die 'Please specify output directory via -o' unless $OUTPUT_DIR;
die "$OUTPUT not a directory"_DIR unless !(-e $OUTPUT_DIR) || -d $OUTPUT_DIR;
  die "$0 requires a directory at $path" unless -d $path;
  die "$0 requires an executable at $path" unless -x $path;
  die "Output directory $out_dir already exists" if -e $out_dir;
  die "$out_dir/$index does not exist" unless -e "$out_dir/$index";
    open(IN, "$find | $grep|") or die "Failed to grep for TODOs: $!";
      die "Bad grep output: $_" unless defined($file);
      die "Malformed task: $task" unless defined($detail);
  open(IN, "<$xml_file") or die "$xml_file: $!";
    die "Malformed $xml_file: $_" unless $testsummary =~ s/>.*//;
    die "Malformed $xml_file: $_" unless $testsummary =~ m/\btests="(\d+)"/;
    die "Malformed $xml_file: $_" unless $testsummary =~ m/\berrors="(\d+)"/;
    die "Malformed $xml_file: $_" unless $testsummary =~ m/\bfailures="(\d+)"/;
  open(IN, "<$html_file") or die "$html_file: $!";
  die "Malformed $html_file: $html"
  die "Malformed $html_file: $summaryTable" unless defined($pct);
  die "exec_log called in wrong context" unless wantarray;
    rmtree $OUTPUT_DIR or die "clean $OUTPUT_DIR: $!";
  open(OUT, ">$out_file") or die "$out_file: $!";
  open(OUT, ">$OUTPUT_DIR/time-series.xml") or die "timeseries.xml: $!";
    open(IN, "<$historical_report") or die "$historical_report: $!";
    die "Malformed historical report $historical_report"
    die "Bad stylesheet filename: $xsl_file"
    die "xsltproc failed on $xsl_file" if $?;
    die "Bad stylesheet filename: $xsl_file"
    die "xsltproc failed on $xsl_file" if $?;

I can't figure out the semantics of our from 
http://perldoc.perl.org/functions/our.html

But the below program:

    sub f() { our $x = 4; print "f: $x\n"; }
    sub g() { our $x; print "g: $x\n"; }
    f();
    g();
    print "main: $x\n";

prints:
f: 4
g: 4
main: 4

So I agree with felix8a that it's not what I want.

Original comment by mikesamuel@gmail.com on 3 Feb 2008 at 10:20

[GoogleCodeExporter]

I forgot to clarify my point about "our". What I meant is that a "my" variable 
in the
global scope shouldn't be used in subroutines.

Anyway, now that Mike has made the change, LGTM.

Original comment by ben@links.org on 12 Feb 2008 at 8:33

[GoogleCodeExporter]

Original comment by mikesamuel@gmail.com on 9 Mar 2008 at 2:51

• Changed state: Fixed

[GoogleCodeExporter]

Original comment by kpreid@google.com on 11 Nov 2013 at 6:46

• Added labels: Component-Test