Re: Restructuring check scripts
Raphael Geissert <atomo64+debian@gmail.com> writes:
> --- a/checks/binaries
> +++ b/checks/binaries
> @@ -21,6 +21,7 @@
> package Lintian::binaries;
> use strict;
> use Tags;
> +use base 'Lintian::Checks';
> use Util;
> use Spelling;
>
> @@ -59,10 +60,10 @@ our %ARCH_REGEX = (
> our $multiarch;
>
> sub run {
> -
> -my $pkg = shift;
> -my $type = shift;
> -my $info = shift;
> +my $self = shift;
> +my $pkg = $self->{pkg};
> +my $type = $self->{type};
> +my $info = $self->{info};
>
> my $arch;
> my $dynsyms = 0;
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.
> 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. 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.
> +sub run {
> + my $self = shift;
> + my $class = ref $self;
> + my @table;
> +
> + eval("\@table = keys %". $class ."::;");
Unnecessary eval, plus this will pick up symbols that aren't methods.
{
no strict 'refs';
my @table = grep { defined &{$class . '::'} } keys %{$class . '::'};
}
> + for my $symbol (@table) {
sort @table. We want to run checks in a predictable order.
> + 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.
--
Russ Allbery (rra@debian.org) <http://www.eyrie.org/~eagle/>
Reply to: