Developers of Sympa

[David Verdin <address@concealed>]

Dear all,

I'll try to give my advice in this conversation.
As a preambule, I'd like to state that I'm aware both of you are far better than me in perl. I'm really thankful that you spend so much time fixing the ugly code I produce on a daily basis. Rest assured that I'm doing my best to improve my code design and my knowledge of perl - I just bought a handful of books that will, hopefully, bear fruits in a few months.

All that to say that I give my advice only because we are speaking about the software I'm in charge here at RENATER, and apologize in advance if I say something dumb.

Le 24/07/13 15:06, IKEDA Soji a écrit :

Hi,

At first, I would like to state that I never commit
anything before David and Etienne consent to finish
ongoing merge.

Thanks Soji. A quick word about the merge: It takes a lot of time because, in sympa-cleanup branch, most files - all perl code - have moved. That means svn merge can't associate the old file location to the new file location. Consequently, we had a lot of tree conflicts (for example: files edited in 6.2 branch whereas it disappeared from sympa-cleanup branch).
Now that we got rid of the tree conflicts - with a single command, as there is no solution in subsversion for tree conflicts - we need to merge files from these tree conflicts one by one. For example, we merge src/sympa.pl.in to src/Sympa/sympa.pl.in.
These merges produce, more or less all the time, textual conflicts. This is due to several reasons:
- big refactoring of the configuration handling in Sympa 6.2,
- big refactoring of function calls (i.e. modules location) in sympa-cleanup,
- fixed indentation in sympa-cleanup.

Most of the time, we need to accept completely one of the branch version, then port manually the modification of the other branch. though we do our best to respect your works, it is likely that we will miss some changes and that the first compilation will produce a lot of errors that we will need to fix.

For now, we still have roughly 40 files to merge. Hopefully, it is only a matter of days before it is finished.

In the meantime, I had a little chat with Guillaume: he works on the new files we created in the 6.2 branch, and on the files for which the merge is over (you can find the files for which conflict is till there by checking the "merge-conflicts" file that is currently at the root of the sympa-cleanup repository). As he created the sympa-cleanup organization, he is best placed to know how to make our recent developments fit in the new code organization. Plus he finds the errors we left while merging.

Anyway, below are my thoughts.

# There is my interim conclusion at end.

On Wed, 24 Jul 2013 11:26:31 +0200
Guillaume Rousse <address@concealed> wrote:

Hello.

I'm a bit concerned by the way object attributes are handled in the 6.2 
branch, and I think getting a consensus here would help solving other 
issues. I'm more and more convinced than the current twisted 
relationships between Site_r,  Site, List and Robot classes is just 
motivated by implementation reasons, rather than modeling purposes.

If I understand correctly what has already been merged in cleanup branch:
- a robot object has two kind of properties:
  + a list defined in Sympa::Site namespace (etc, home, name), called 
'attributes'
  + a list defined in Sympa::Site namespace (blacklist, ...), plus 
another list in confdef namespace, called 'parameters'
For each of those properties, a single read/write accessor is 
automatically generated through AUTOLOAD mechanism

- a site class has a single type of properties, called 'attributes', 
defined in Sympa::Site namespace (auth_services, etc...). For each of 
those properties, a single read/write accessor is automatically 
generated through AUTOLOAD mechanism.

Here a my concerns.

First, we have a vocabulary issue: what is an object attribute ?
Here is is used as 'some kind of object property which is not a 
parameter', whereas for me, it is a plain synonym of 'object property'.
For me, everything with an accessor is an object attribute, wether its 
value comes from a configuration file, is internally generated, etc...

Currently, there are at least three categories of "attributes":

(a) Mandatory paremeters of object, e.g. "domain" for Sympa::Robot
   object.
(b) Configuration parameter values related to the object.
(c) Values derived from parameters above, e.g. "sympa".

I suppose (c) would be implemented by another way. Similarly,
other kinds of data structures such as "robot_by_http_host"
should be handled in different manner.

So problem will be: What may we call (a) and (b)?

I'm not sure to understand correctly. Does it mean that the difference between (a) and (b) is that (a) are mandatory?
From my point of view, when manipulating data from a robot or a list, we don't care where they come from, or if they are mandatory or not. We just need accessors to whichever data we need on the moment.

Guillaume added:

All our classes have mandatory (or not) properties defining them:
  - a name for a list or a robot
  - a subject for a message
  - a file for a lock
  - a type for a database
  etc...

That's category (a). Let's call them object attributes, and let's use accessors for them. Wether we use get_foo()/set_foo() or foo() model, as well as wether they are to be statically or dynamically generated, is a separated question.

Now, some of our classes, representing configuration elements (site, robots), also have a huge list of configuration directives attached to them:
  - a database name for a site
  - a blacklist for a robot
  - etc...

That's category (b). Let's call them parameters, and as they are a quite large number of them, let's manage them through a generic get_parameter() method, instead of specific accessors.

The point is than the database name, which is an attribute for the database object, should not mandatorily be an attribute for the object representing the configuration part where it is defined. And in fact, not treating them as attributes would leverage the rest of the discussion.

For category (c), let's consider them as attributes, as per category (a). That's the usual class-school discussion about get_x()/get_y() accessors for a point object, wether they are mapped over actual cartesian coordonates, or computer from radial coordonates.

What's about this proposal ?

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?

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.

David

--
A bug in Sympa? Quick! To the bug tracker!

David Verdin Infrastructure pour les Services Informatiques
Tél : +33 2 23 23 69 71 Fax : +33 2 23 23 71 21   www.renater.fr	RENATER 263 Avenue du Gal Leclerc 35042 Rennes Cedex

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