Code review: code-dashboard-initial-20-Jan-2008 about caja HOT 6 CLOSED

Comment #1 originally posted by [email protected] 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.

from caja.

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.

from caja.

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 "$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.

from caja.

Comment #4 originally posted by [email protected] 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.

from caja.

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

<empty>

from caja.

Comment #6 originally posted by [email protected] on 2013-11-11T18:46:13.000Z:

<empty>

from caja.