Developers of Sympa

[Guillaume Rousse <address@concealed>]

Le 25/07/2013 14:46, David Verdin a écrit :
[..]
> OK. So basically, you wonder which "element-describing-an-object" should
> be accessed through a sub named after it?
> If clarity is the main object here, then let's spare the element in tqo
> categories:
> (a) the one you can define in the main configuration files (either
> robot.conf, sympa.conf or config for lists) which are defined in either
> listdef.pm or confdef.pm
> (b) all the rest. blacklist, log_condition, auth mechanims, etc.
> Whatever is not read directly in a file (or in default values).
>
> Let's define a generic mechanism for category (a), whichever you prefer,
> that will avoid any ambiguity with category (b).
> For category (b), we can use explicit getters and setters at the highest
> pertinent level. These getters and setters will have names that can't be
> confused with those used for category (a). The getters and setters for
> (b) already exist somewhere in the code. We just hae to retrieve them
> and put them in the right place.
>
> There. Problem solved?
It seems so. Let's try a slight reformulation:
- all objects have attributes, accessed through a pair of 
get_foo()/set_foo() accessors
- some objects (List, Robot, Site) additionaly have read-only 
configuration parameters, which are simple key/values pairs, accessed 
through a unique get_parameter() method.

> > > Second, the spreading of attribute definition breaks object-modeling
> > > assumption than classes should be self-contained: the list of attributes
> > > for a robot object should be defined in Sympa::Robot object, not
> > > somewhere else (its specific attributes, of course, inherited ones being
> > > defined in its parent classes)
> Well at first, with the new code written by Soji, I was confused. I
> didn't know where the subs used were defined. But then I found really
> easy to have only a small sub to call to access a config parameter.
I have no objection having a method defined only once in a parent class. 
That's one of the basic ideas of inheriting from another class.

But that's only true if this definition is actually unique, and is not a 
giant switch enumerating all possibles subclasses, as most methods in 
Site and Site_r actually do. For instance, here is the dynamically 
created accessor in Sympa::Site:

# common case

if (ref $self and ref $self eq 'Sympa::Robot') {
  # Sympa::Robot specific case
} elsif ($self eq 'Sympa::Site') {
  # Sympa::Site specific case
} else {
  croak "fatal error";
}

That's exactly the same kind of pattern that was previously used in 
Sympa::Database::connect() method, to handle database-specific pre or 
post-connection handling. And this is plainly wrong, because it 
introduces a strong coupling between the parent class and the child 
class. The parent should only define its own behaviour, and the child 
should override the method to enforce its own.

Here is a better alternative:

package Sympa::ConfigurableObject;

=name1 An abstract class for all classes using configuration parameters

sub get_parameter {
    my ($self, $parameter) = @_;

    # check the parameter name
    my $class = ref $self;
    if (! $class::params{$parameter}) {
        # handle 'no such parameter' case
    }

    # check initialisation status
    if (! $self->{parameters}) {
        # handle 'uninitialized' case
    }

    return $self->{parameters}->{$parameter};
}

package Sympa::List;

use base qw(Sympa::ConfigurableObject);

# flat (but ordered) list of parameters
our @parameters = (
    {
        name       => subject,
        group      => 'description',
        gettext_id => "Subject of the list",
        format     => '.+',
        occurrence => '1',
        length     => 50
    },
    ...
);

# the same list, indexed by parameter name
our %parameters = map { $_->{name} => $_ } @parameters;

package Sympa::Site;

use base qw(Sympa::ConfigurableObject);

etc...


This way:
- each class in self-contained (parameters for Sympa::Site are defined 
in Sympa::Site namespace, parameters for Sympa::List are defined in 
Sympa::List namespace, etc...), but still accessible from outside if needed.
- the base class has a human-understandable name
- get_parameter() if needed is defined only once, but could eventually 
get overriden if needed
- this is far more readable than current overcomplex AUTOLOAD stuff

The last point does matter: we are supposed to make Sympa code more 
accessible for everyone, in order to make the contributors pool larger, 
not resort to Perl black magic just for syntactic sugar or to spare a 
few lines of code.

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