Developers of Sympa

[IKEDA Soji <address@concealed>]

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.

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?

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

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


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.

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.


Thanks,

--- Soji

> Cheers,
> 
> David


-- 
株式会社 コンバージョン  セキュリティ&OSSソリューション部   池田荘児
〒231-0004　神奈川県横浜市中区元浜町3-21-2 ヘリオス関内ビル7F
e-mail address@concealed  TEL 045-640-3550
http://www.conversion.co.jp/