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

[kpreid]

Original issue 45 created by mikesamuel on 2008-01-23T22:31:29.000Z:

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.

[kpreid]

Comment #1 originally posted by ben@links.org on 2008-02-01T11:49:16.000Z:

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.

[kpreid]

Comment #2 originally posted by felix8a on 2008-02-01T12:22:25.000Z:

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.

[kpreid]

Comment #3 originally posted by mikesamuel on 2008-02-03T10:20:59.000Z:

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 "$outdir/$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 $xmlfile: $" unless $testsummary =~ s/>.*//; die "Malformed $xmlfile: $" unless $testsummary =~ m/\btests="(\d+)"/; die "Malformed $xmlfile: $" unless $testsummary =~ m/\berrors="(\d+)"/; die "Malformed $xmlfile: $" 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.

[kpreid]

Comment #4 originally posted by ben@links.org on 2008-02-12T20:33:25.000Z:

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.

[kpreid]

Comment #5 originally posted by mikesamuel on 2008-03-09T02:51:44.000Z:

<empty>

[kpreid]

Comment #6 originally posted by kpreid@google.com on 2013-11-11T18:46:13.000Z:

<empty>