Developers of Sympa

[Guillaume Rousse <address@concealed>]

Le 01/06/2013 03:12, IKEDA Soji a écrit :
> I think interfaces of each spool is independent from its backend.
Yes, that's the point of defining the interface at the abstract class 
(Sympa::Spool) level.

package Sympa::Spool;

sub get_message {
    croak 'not implemented';
}

package Sympa::Spool::File;

use base 'Sympa::Spool';

sub get_message {
    // do stuff;
}

Whereas the delegation pattern also allows this kind of construct, it is 
also slightly more complex, with two different classes hierarchies, and 
slightly less efficient, as every method call has to be proxied to the 
delegate instance:

package Sympa::Spool;

sub get_message {
    my $self = shift;
    $self->{backend}->get_message(@_);
}

My main point here is consistency with other parts of the code, for 
instance Sympa::Datasource::SQL. We have a factory method in the 
top-level class, returning an instance of a database-specific subclass:

my $source = Sympa::Datasource::SQL->create(
    name => 'foo',
    user => 'foo',
    host => 'localhost',
    type => 'mysql'
);
$source belongs to Sympa::Datasource::SQL::Mysql class.

Applied to Spool case:

my $spool => Sympa::Spool->create(
   name => 'incoming',
   type => 'file'
);
$spool belongs to Sympa::Spool::File class.

> For example by my idea,
>
>    use Spool;
>    ...
>    $incoming_spool = Spool->new('incoming');
>    $message = $incoming_spool->get_message();
>    ...
>    $pending_spool = Spool->new('pending');
>    $pending_spool->store($message);
>    ...
>    $message = $pending_spool->get_message();
>    ...
>
> "incoming" spool may use "msg" storage (directory) by "File"
> backend, "pending" spool may use "mod" storage (table rows) by
> "SQL" backend, ... or they may not.  Details were given by Spool
> package only at once.
I still disagree with hardcoding specific spool details in the Spool 
class. They should be either in the configuration, for user-modifiable 
parameters such as backend type, and in the executable, for others.

--
Guillaume Rousse