Developers of Sympa

[David Verdin <address@concealed>]

Hi, it's me again,

Le 29/07/2013 15:05, Guillaume Rousse a écrit :

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.

Yes, though some values are actually hashes or different structures.
And I need to read Soji's answer about his development logic to fully understand the problematic.

address@concealed">

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.

Yes, actually the work in the Database modules was not finished. I had time to handle sub-module selection, queries, and wanted to handle all the parts that were RDBMS-specific and move them to their dedicated package.
You rewrote the whole thing before, so now we will do it your way - which is also good from my point of view.

address@concealed">
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

I like the concept and, as Soji pointed it, it would certainly the good branchment point where to put the handling of several configuration formalisms.

address@concealed">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.

I don't think this was Soji's idea. He's just good at perl and can write code using the AUTOLOAD mechanism - which I certainly could not - well I can now, but I didn't even know it existed before I saw Soji's code.
Perl is just this way: Developpers have a large spectrum of techniques available to achieve a single goal. As a long-term integrator of other people's code in Sympa, I am used to seeing this mosaic of coding styles and just accepted it. That's why I didn't object to the architecture Soji proposed to the SIte/Robot/List set.

But as the core developpers, it is important that we agree upon coding conventions - a minimal set of such at least.
As a non-native perl coder, I must say that AUTOLOAD was troubling at first but once I admitted the mechanism ad understood the coding comfort it provided, I found it quite cool. Soji and Guillaume are more used than I am to perl arcanes and to perl's community.
Soji thinks that there is no problem with the current organization, Guillaume thinks there is.

I agree to the understanding problematic: One willing to code something in Sympa must be able to use parameter / attributes easily, i.e. find easily what parameters are available, and how to call them. The distinction between configuration-defined and computed parameters is essential.
If we agree on this, I let you two choose chichever naming pattern you find is better. As a former Java developper, I must say that I like the get_foo / set_foo system for its clarity.

Cheers,

David