Re: Restructuring check scripts

[Russ Allbery <rra@debian.org>]

Raphael Geissert <atomo64+debian@gmail.com> writes:
> Russ Allbery wrote:

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

I suspect it's a wash from a performance standpoint, and I think it's a
bit clearer to keep passing in the arguments to the run script.

>>> 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()?

More that you can get rid of the object completely and just do the direct
sub calls that the run method was doing, I think (although it's been a
while since the thread, and I should have responded right away so that I
remember what I was thinking).

>>> +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.

Well, usually using string eval is considered much more problematic than
disabling strict references.  Disabling strict references bypasses a bit
less in the way of checking.  (Plus, yeah, performance.)

>> 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 :)

True.  :)  But since disabling strict references avoids that problem too,
I thought I'd mention it.

>> 
>>     {
>>         no strict 'refs';
>>         my @table = grep { defined &{$class . '::'} } keys %{$class .

> "my"? I don't think it would be of any use then ;)

Whoops.  :)

> Oh, and "defined &{$class . '::' *. $_*}" :)

Oh, yes, sorry.

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

Consider, for instance, checks for alternative output formats, where
sorting the Lintian output can make the tags file in the Lintian test
suite unreadable.  (Right now, by chance all of our output formats use one
line per tag, but I expect that to change eventually for at least the XML
output method.)

Also, whenever there are problems, it's a lot easier to track them down if
the tests are run in the same order for us as they are for the person
reporting a problem.

-- 
Russ Allbery (rra@debian.org)               <http://www.eyrie.org/~eagle/>