Developers of Sympa

[David Verdin <address@concealed>]

Hi guys,

I'm re-acclimating to work while still on holidays. I'll try to keep in touch with you until September 9th, when I officially come back to work (but as I'm alone with my two kids, this won't be always easy).

Le 31/07/2013 18:25, IKEDA Soji a écrit :

On Wed, 31 Jul 2013 15:14:28 +0200
David Verdin <address@concealed> wrote:

Hi, it's me again,

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

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.

In my understanding, the function of accessors is to hide in-core
processing (caching, delayed loading, seialization/deserialization
etc.) and to present as if programmers are directly accessing to
higher-level objects.

Completely. Developpers don't need to wonder where they take data from. They just need accessors - and setters - and they probably need only one formalism.
Actually, I start wondering whether we shoul not have a single formalism for all parameters (i.e. a single formalims, for categories a and b defined above).
That would make it very simple for developpers to develop functionnalities without having to wonder how to call them.

So whether style they have, the logic they use will be similar as
AUTOLOAD() I wrote (except autoloading mechanism itself).  I believe
there are no dessension on this point.

A few comments:

- David's proposal seems to omit values I called "mandatory
  parameters" ($robot->domain etc.), and also general cases for the
  other objects.  How naming convention for them are proposed?

Actually, I think I don't understand the semantics below "mandatory parameters".
What is the difference between, say $robot->domain and $robot->color_2 ?

- Configuration parameters probably can not be read-only in the
  future.
  In fact, by now in-core cache of List configuration is used to
  save values into config files.  Why don't we implement setters
  for them?

I agree. This is completely in accordance with our SaaS project, in which listamsters will have to be able to change any parameter through the web interface.

- Nevertheless, many objects probably have only getters.  Hence
  concept of "getter/setter pairs" can be inadequate.

  For example, see the use of get_envelope_sender() in
  Message::load() on sympa-6.2-branch.  It may have other
  action prefix such as "load_", "compute_", "initialize_" etc.,
  because it tends to set in-core cache.  In other place
  "set_pid_file()" are used to get PID file name (they had been
  fixed by Guilaume), probably due to similar confusion.

Or simply to historical accumulation of different naming conventions... ;-)
I think setters/getters pairs make sense, even though it will imply different programmatic logic from a paramter to another.

There is one more comment below.

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.

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.

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.

# I guess unconfortableness on "get_" prefix is not due to
# implementation.  But I don't wish to begin a lecture of
# aesthetics here.

Same here. As long as the naming convention is clear, it's OK in my book.

I'm understanding that consensus building is not a kind of voting.
It is not always neccessary to persuade opposers.  Instead, let's
proof your belief on the running code.

However, we should promise: our convention binds only us.  We should
not blame contributors not following our rules.  Reformatting is our
work.  We must not blame anyone.

Obviously. We want to help people developping in Sympa, not force them into our thinking framework. It's hard enough to find a consensus with only the three of us, I don't want to enter this kind of discussion with tne - let's dream - or more developpers.
However, we can still give development tips on the developers page of Sympa.

Cheers,

David

Thanks,

--- Soji

Cheers,

David