Developers of Sympa

[Guillaume Rousse <address@concealed>]

Le 12/08/2013 12:36, IKEDA Soji a écrit :
> > > I'd like to propose as following:
> > >
> > > o Spool class would be handled in single style:
> > >
> > >       my $spool = Sympa::Spool::Digest->new();
> > >       my $spool = Sympa::Spool::Incoming->new();
> > >       my $spool = Sympa::Spool::Task->new();
> > >       my $spool = Sympa::Spool::Archiving->new();
> > 'Task' and 'Digest' are nouns, whereas 'Archiving' and 'Incoming' are
> > not, this is not consistent. I guess incoming spool is meant to store
> > messages, so that should rather be:
> > my $incoming = Sympa::Spool::Message->new();
>
> Please give appropriate names.  "Incoming" spool contains messages
> sent from outer world.  "Archiving" spool contains messages to be
> added to archives.
You're confusing instance and classes names here, and I'm precisely 
trying to give classes appropriate names, according to what they store.
If both incoming and archiving spool contains messages, they should be 
instances of the same Sympa::Spool::Message class, ie:
my $digest    = Sympa::Spool::Digest->new(...);
my $task      = Sympa::Spool::Task->new(...);
my $incoming  = Sympa::Spool::Message->new(...);
my $archiving = Sympa::Spool::Message->new(...);

That is a coherent naming scheme.

> > > o Each class is a subclass of common abstract class (i.e.
> > >     Sympa::Spool) and has following variations:
> > >
> > >      (1) Content object to handle (message digest, Message, Task, ...),
> > >        defined by each class itself.
> > >      (2) Physical backend (SQL, filesystem, ...) defined by configuration
> > >        option.
> > >      (3) Domain in the backend (on SQL, value of name_spool column;
> > >        on filesystem, name of subdirectories: "digest", "msg", "task",
> > >        ...) defined by each class itself.
> > It would be clearer to enumerate the needed classes, and to define their
> > exact relationships. For instance, how will you manage backends ? Yet
> > another class hierarchy ?
>
> Of course, another class will be used.  I have not yet understood why
> you are rejecting "two class hierarchies" idea.
>
> > And I still object having spool classes hardcoding their settings, which
> > tends to multiply artificially the number of classes needed. If you
> > hardcode the fact than spool 'X' uses domain 'X' in a Sympa::Spool::X
> > class, than you'll need as much classes as you have spools. For me,
> > those settings should be passed at instanciation time by calling code, ie:
> >
> > my $incoming = Sympa::Spool::Message->new(
> >       storage => $configuration->get_param('spool_storage'),
> >       domain  => 'incoming',
> >       name    => 'incoming'
> > );
>
> "name" looks duplicate of "domain".  So if I wrote in similar style:
>
> my $incoming = Sympa::Spool->new(
>      generator => 'Sympa::Message',
>      storage => 'Sympa::Spool::Backend::SQL', # taken from 
> incoming_spool_storage
>      domain  => 'msg',                        # name of spool for incoming 
> messages.
> );
What it is supposed to return ?

If that's a 'Sympa::Spool' instance, that's OK, excepted than in this 
case you don't need 'Sympa::Spool::Message' class, and than 
'Sympa::Spool::Backend::SQL' could as well get renamed to 
'Sympa::Spool::SQL'.

If that's an instance of 'Sympa::Spool::Message', that's also OK, but 
I'd prefer the method used here to be renamed from 'new' to something 
else, to clearly mark a distinction between a constructor (a method 
returning an instance of the class it is invocated on) and a factory (a 
method returning an instance of another class it is invocated on)

See what I've done for databases subclasses:
- Sympa::Database->new() should fails, as it is an abstract class.
- Sympa::Database::MySQL->new() returns a Sympa::Database::Mysql instance
- Sympa::Database->creates(type => mysql) returns a 
Sympa::Database::Mysql instance

> > > o Configuration parameters for (2) above would be defined for
> > >     each spool.  Current possible values are "sql" and "filesystem".
> > >
> > >      digest_spool_storage
> > >      incoming_spool_storage
> > >      task_spool_storage
> > >      ...
> > >
> > > Any comments?
> > Is it really needed to mix spool storage type ? What about first getting
> > a simple, but working implementation, before reaching into unproven
> > complexity ?
>
> As I remember ---
> - In sympa-6.1, filesystem-based spool were used for all spools
>    except for bulk sending.
> - Then in sympa-6.2-branch, there was an attempt to switch almost
>    all spools to SQL-based.
> - However, this plan seems to have been partially changed (I don't
>    excactly know why).
It didn't supported the load, AFAIK. And the current plan was to allow 
both types, with a single configuration switch (ie, all file-based, or 
all sql-based spools).

--
Guillaume Rousse
INRIA, Direction des systèmes d'information
Domaine de Voluceau
Rocquencourt - BP 105
78153 Le Chesnay
Tel: 01 39 63 58 31

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