Hi,
Russ Allbery wrote:
> Raphael Geissert writes:
[...]
> Why did you move the arguments to the run method into object data? I
> think they're better left as arguments that are passed to every check
> function. It's one fewer level of indirection.
I though setting up once and calling many was better. I didn't like the
amount of changes needed, but I decided to go on and see what you'd say.
>
>> diff --git a/lib/Checker.pm b/lib/Checker.pm
>> index 7aa7e23..642204f 100644
>> --- a/lib/Checker.pm
>> +++ b/lib/Checker.pm
>> @@ -40,7 +40,9 @@ sub runcheck {
>> require "$LINTIAN_ROOT/checks/$name";
>>
>> $name =~ s/[-.]/_/g;
>> - eval { &{'Lintian::'.$name.'::run'}($pkg, $type, $info) };
>> + my $check;
>> + eval "\$check = Lintian::$name->new(\$pkg, \$type, \$info)" and eval
>> {$check->run()};
>
> You can use the same method in the code that you're replacing to avoid
> the string eval.
Indeed, I don't know why I didn't notice it.
> However, I don't think the parent class here is
> useful; I would just move the run code from below directly into
> Checker.pm rather than creating the parent object.
>
You mean blessing the object directly in run()?
>> +sub run {
>> + my $self = shift;
>> + my $class = ref $self;
>> + my @table;
>> +
>> + eval("\@table = keys %". $class ."::;");
>
> Unnecessary eval
I prefer the cleaner approach without disabling strict references; but am
going to do it without the eval only because of the performance penalty.
> plus this will pick up symbols that aren't methods.
I don't expect it to have any side effect since a) there's not much in
object's symbols table, b) it would be less likely for a variable to be
called check_*, and c) well, it can be mitigated :)
>
> {
> no strict 'refs';
> my @table = grep { defined &{$class . '::'} } keys %{$class .
"my"? I don't think it would be of any use then ;)
Oh, and "defined &{$class . '::' *. $_*}" :)
> '::'};
> }
>
Okay, using that.
>> + for my $symbol (@table) {
>
> sort @table. We want to run checks in a predictable order.
Why? Anything relying on a predictable order is likely to be bogus.
That's why for example the tests use sort(1) on lintian's output.
>
>> + next unless $symbol =~ m/^check_/;
>> + $self->$symbol;
>> + }
>> + 1;
>> +}
>
> Please use an explicit return as a matter of style rather than falling
> off the end of the sub and returning the last evaluated value.
>
Okay.
By the way, sorry for my recent behaviour/mood, I had some RL issues.
Cheers,
--
Raphael Geissert - Debian Maintainer
www.debian.org - get.debian.net
Attachment:
lintian-checks.mbox
Description: application/mbox