Developers of Sympa

[IKEDA Soji <address@concealed>]

Hi,

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

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


> 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)
> 
> Third, it clutters our API. I'm currently counting 17 public functions 
> or methods in Sympa::Robot. Automatically generating methods for each 
> element of confdef::params will add 258 additional methods...

Methods never increase endlessly.  Fathermore, their
definition ends only at once: "They are configuration parameters".

It is not always necessary to stick to treat them as subroutines
(functions), as constants are not usually treated as subroutines.


> Fourth, it breaks another good practice, which considers than each 
> method name should be following an explicit naming rule, the more usual 
> being 'action_object' (for instance "send_message", "execute_query', 
> etc...). What does means $robot->main_menu_custom_button_3_target() ?
> 
> Moreover, those accessors being read only, they'd better get named 
> 'get_something()' than 'something()'. You don't need to be a genious to 
> understand than there is something wrong in 
> '$robot->get_home("/var/lib")', whereas '$robot->home("/var/lib")' is 
> more ambiguous.

This is rather a matter of preference, I suppose.

Previous notation was:
  Sympa::Configuration::get_robot_conf("robot.name", "home")
Someones prefer to:
  $robot->get_conf("home")
or:
  $robot->get_home()
Others do:
  $robot->home

I love the last notation without redundancy: I feel "get_"
is not required because it is obvious from the context.


> Fifth, it make use use AUTOLOAD, just to automatically generate those 
> additional methods. AUTOLOAD was supposed to be part of solution, I 
> think it is becoming the main problem in new code:
> - useless complexity: AUTOALOAD method in Sympa::Site is unmaintainable, 
> especially without related tests
> - unwanted side-effect: DESTROY method has to be explicitely defined, 
> even without content, just to avoid side-effect

Unmaintainable? Indeed, current Sympa::Site::AUTOLOAD includes
slightly complex if-statements, it is partly due to ad hoc data
structres I described above.

If you mean of unit test, there are no need to prepare
test cases for all of each configuration parameters.

Use of empty DESTROY is simply a idiom, as I said some months ago.
It may be hidden by using any of modules, if necessary.


> So, my current proposal would be:
> 1) find another name for 'the kind of object property which is not a 
> parameter', instead of attribute.
> 2) stop generating accessors dynamically, and stay with a single 
> get_parameter() method. Eventually, another method could be needed if we 
> really have two different type of attributes, and they need to be 
> handled differently
> 3) find some way to enclose attributes definitions inside classes 
> definitions
> 
> Also, the goal is to be consistent, both in our implementation, and in 
> our API, across all our clases, not just Sympa::Robot and Sympa::List. A 
> database object, has statically-defined get_type(), get_name(), 
> get_host() and get_user() accessors, for instance. Why should we manage 
> them differently other classes ?

My comments are as above.


Anyway, if I must give a matter of priority, I choose to
release the code working more or less.  To be frank, style may be
changed later.  I would like the merge to finish earlier
then to run it.

Sorry for possible impolite words due to my poor English.


Regards,


-- 
IKEDA Soji <address@concealed>