Developers of Sympa

[David Verdin <address@concealed>]

Hi all,

Le 18/12/12 14:02, Guillaume Rousse a écrit :

address@concealed">Le 17/12/2012 09:18, address@concealed a écrit :

Revision
    8261
Author
    sikeda
Date
    2012-12-17 09:18:15 +0100 (lun. 17 déc. 2012)


      Log Message

[dev] split AUTOLOAD of Site to Site and Robot,
to prevent Site accessors overriding Robot accessors.

I'm not a big fan of AUTOLOAD ... It defeats static analysis, either by dedicated tools such as Test::Pod::Coverage, or just by running grep in source tree. It also often has side-effects, such as the need to define an empty DESTROY method to avoid it being called when an object is garbage-collected, for instance.

However, I understand we have quite a large number of attributes (despite I don't get the distinction between 'attributes' and 'parameters' later in your code), and that it would be painful to manually repeat an attribute for each of them. So, I could live with it, especially as I have no better alternative to suggest :)

However, I'd really prefer all those read-only accessors being called get_foo/bar instead of just foo/bar:
- it would be more consistent with other piece of code (see Sympa::VOOT::Provider, for instance)
- a get_foo() has an explicit read-only semantics, whereas a foo() method raising an exception if an argument is passed is clearly a trap
- some of those attribute names sound much like a verb than a noun:
$robot->request(), for instance, is quite unfortunate, and suggest an action

I'm also quite found of the getters / setters. But we have to deal with legacy. What Soji proposes is a good compromise for the 6.2 version. It gives us accessors without having to rewrite large parts of the code.
However, using get_* and set_* methods implemented looks like a good aim for 6.3 on my opinion.

What would be great, would be to have generic get_ and set_ method for each object, having all the same behaviour and, in addition, get_foo and set_foo methods that would replace the generic behaviour for parameter foo when additional tests would be required.
Cheers,

David

address@concealed">

+sub AUTOLOAD {
+    Log::do_log('debug3', 'Autoloading %s', $AUTOLOAD);
+    $AUTOLOAD =~ m/^(.*)::(.*)/;
+    my $pkg  = $1;
+    my $attr = $2;

-=item listmasters
+    my $type = {};
+    ## getters for site/robot attributes.
+    $type->{'RobotAttribute'} =
+    scalar(grep { $_ eq $attr } qw(etc home name));

+    ## getters for site/robot parameters.
+    $type->{'RobotParameter'} = scalar(
+    grep { $_ eq $attr }
+        qw(blacklist list_check_regexp loging_condition loging_for_module
+        pictures_path request sympa)
+    ) ||
+    scalar(grep { !defined $_->{'title'} and $_->{'name'} eq $attr }
+        @confdef::params);

Implementation remarks...

First, the use of scalar() is useless here, as turning an empty list to 0 doesn't change much for later evaluation in boolean context a few lines thereafter.

Second, this whole block of code would be more readable by first computing the list of available attribute names, then using a single $type variable to store the lookup result.

# generate list of available class attributes
my @attributes = qw(etc home name);
my @parameters =
     qw(blacklist list_check_regexp loging_condition loging_for_module
        pictures_path request sympa),
     map { $_->{'name'} } grep { !$_->{'title'} } @confdef::params

# which type of method do we need here ?
my $type =
     grep { $_ eq $attr } @attributes ? 'attribute' :
     grep { $_ eq $attr } @parameters ? 'parameter' :
                                        undef;

Last, if this code is supposed to be called multiple times, it would be more efficient to index those values once for all instead of enumerating them each time.

package Robot;

# index available attributes and parameters names
my %attributes = map { $_ => 1 } qw(etc home name);
my %parameters = map { $_ => 1 } (
     qw(blacklist list_check_regexp loging_condition loging_for_module
        pictures_path request sympa),
     map { $_->{'name'} } grep { !$_->{'title'} } @confdef::params
);

sub AUTOLOAD {
...
if      ($attributes{$attr}) {
...
} elsif ($parameters{$attr}) {
...
} else {
    croak "Can't locate object method \"$attr\" via package \"$pkg\"";
}
}

Attachment: smime.p7s
Description: Signature cryptographique S/MIME